On Tuesday 19 January 2016 08:14:59 Dave Chinner wrote:
On Mon, Jan 18, 2016 at 08:53:22PM +0100, Arnd Bergmann wrote:
On Monday 18 January 2016 09:40:12 Deepa Dinamani wrote:
On Mon, Jan 18, 2016 at 2:56 AM, Arnd Bergmann arnd@arndb.de wrote:
I agree it's ugly and fragile to have one huge patch,
Nobody is suggesting one huge patch here. This can all be done with small steps.
but I think the best way to illustrate it is to make it as small as possible and then talk about whether that makes it acceptable or how we can work around the problems.
Do you have an estimate what portion of the file systems need any changes at all before we can flip over VFS to the new types?
All filesystems will, at least, need auditing. A large number of them will need changes, no matter how we "abstract" the VFS type change, even if it is just for 32->64 bit sign extension bugs.
Filesystems that have intermediate timestamp formats such as Lustre, NFS, CIFS, etc will need conversion at the vfs/filesytem entry points, and their internals will remain unchanged. Fixing the internals is outside the scope fo the VFS change - the 64 bit VFS inode support stops at the VFS inode/filesystem boundary.
What I meant with "one huge patch" is simply just the change that is needed to modify the type, if we don't use conversion helper functions.
If it's less than half, we you can try yet another variation (nothing new really, we are always dealing with the same few tricks):
- add timestamp range checking and clamping
- kill off CURRENT_TIME
Other way around. First make everything use the existing current time functions, then ensure that incoming timestamps are truncated correctly, then add range checking and clamping to the existing time modification functions.
Makes sense.
- for each file system that uses struct timespec internally to pass around inode timestamps, do one patch that adds a timespec_to_inode_time() and vice versa, which gets defined like
static inline struct timespec timespec_to_inode(struct timespec t) { return t; }
This works, and is much cleaner than propagating the macro nastiness everywhere. IMO vfs_time_to_timespec()/timespec_to_vfs_time would be better named as it describes the conversion exactly. I don't think this is a huge patch, though - it's mainly the setattr/kstat operations that need changing here.
Good idea for the name.
If you are ok with adding those helpers, then it can be done in small steps indeed. I was under the assumption that you didn't like any kind of abstraction of the type in struct inode at all.
- change the internal representation in one patch that changes those helpers along with the struct members.
If you are talking about converting internal filesystem representations to (e.g. CIFS fattr, NFS fattr, etc) then this is wrong. Those filesystems are isolated and able to use timespecs internally by step 3, and without protocol/format changes can't support y2038k compliant dates. Hence fixing such problems is a problem for the filesystem developers and is not an issue for the VFS timestamp conversion.
No, once we have the timespec_to_vfs_time helpers in all file systems, that change is just for VFS, and should not touch any file system specific code.
It is the equivalent of patch 8/15 in the current version of the series, except that it changes one version of the code to another rather than changing a CONFIG_* symbol that alternates between the two versions coexisting in source.
When I first attempted the conversion, I ended up with a very similar trick that Deepa has now, and it's very helpful to find what the code locations are that need to be touched, without doing them all at the same time, as you can simply flip that option to try out another file system.
However, I agree that this is better not reflected in how the patches get applied in the end, and there is no need to clutter the git history with having both options in the code at the same time, and we should try to avoid touching a lot of code more than once wherever possible.
- change the file systems to use timespec64 internally instead of timespec.
I think that will work and leave use with a relatively clean code base, as well as be able to address y2038k support each individual filesystem in our own time.
Ok, thanks.
Arnd