On Monday 14 December 2015 19:20:24 Deepa Dinamani wrote:
On Mon, Dec 14, 2015 at 3:52 AM, Arnd Bergmann arnd@arndb.de wrote:
On Friday 11 December 2015 20:47:02 Deepa Dinamani wrote:
And based on if anybody rearranges these fields while adding new ones, we might have different kind of packing. While the struct is guaranteed to have 12 bytes always.
The inode structure is generally highly optimized, we wouldn't mindlessly rearrange things in general. We can definitely try out both, and it's easy to change afterwards when more comments come in, as long as the accessor is able to cope with either format.
Ok. In that case I can change it to be this way. If we change only the first field to be packed, this would also be a hack since we know that there will be three of them. Making the whole struct packed could take more performance hit as you said. I did not check if compiler could guess the size of second field because of aligned. I'm yet to do more research/ look at disassembly.
The only advantage of the structure is grouping of the two fields: sec and nsec together. So I would say it increases readability.
But, there are filesystems that access them this way as individual fields. So we could pick performance in this case?
In any case, we should only ever need to mark the structure as __attibute__((packed, aligned(4))), and the effect of marking the first member or the entire struct is exactly the same then.
Yes, that would be one way to handle it. As long as we have a macro or inline function abstraction, we can hide it in there.
Not needing macros might actually be nice too, so we could have a set of inline functions instead, like
static inline struct timespec64 inode_get_atime(struct inode *inode) { return (struct timespec64) { .tv_sec = inode->i_atime_sec, .tv_nsec = inode->i_atime_nsec, }; }
static inline void inode_set_atime(struct inode *inode, struct timespec64 *t) { inode->i_atime_sec = t->tv_sec; inode->i_atime_nsec = 0; }
There are only three of them, so it's not much more code than the macros, and those are certainly more readable and easier to extend.
I can go ahead and change it to use only inline functions instead of macros. I was thinking of this:
static inline struct timespec64 inode_get_inode_time(struct inode_time atime)
These can have definitions similar to that of inode_time and ts64 conversions I have now.
This would replace the inode_time_to_timespec64() function, and have the same definition, right?
I will pair them with the inode struct as well and not put them in time64.h.
It's nice how you get around naming the specific time field with a string concatenating macro like this, but I wonder if we also need to pass the inode here. We probably need to pass the inode or the superblock in the place where we assign the time, to be able to limit the range and resolution to the file system's capabilities, but I don't know if we also might need the inode when getting the data out again. Probably not, but we should think about it some more (and not add it unless we believe that it's actually needed).
I was thinking if there is a way to only use current_fs_time() to assign times and all the range checking could be done this way. I was able to do this in all of vfs code except one function: simple_set_acl() This I'm still looking into.
I think simple_set_acl() is wrong, it should use current_fs_time() as well, and that is a patch we probably want to merge independently.
I don't know if range checking is needed for get functions. It might only be needed if it is updated between set and get. I will check if this is possible.
I've been thinking about it some more. We clearly don't need a resolution or range check when reading the inode from disk, as there won't be any out-of-range values by definition. We also don't want any timestamps to appear in the inode structure that are different after writing to disk and reading it back, and I checked that all file systems that implement their own getattr function just copy the fields from the inode into kstat, so it has to be done when setting the inode fields.
There may be a few more files that currently don't check the resolution when assigning the current time in the setattr function, or when storing the current time, and we should fix them as we get there to do both checks.
Two more thoughts:
I keep thinking that we need to do the same change for all three structures (inode, iattr, kstat), so we only have to modify each file system once. You currently use timespec64 in iattr and kstat, which is nicer in principle, but it means that we need an extra set of abtractions to read/write those. If we go with your inode_time method, it's probably best to do it in all three structures, while using separate fields, I guess we would need an extra layer of macros around them.
I do not plan to use macros for iattr and kstat. This I kind of got lucky because it uses same field names as timespec ( it is timespec in 64 bit arch).
As I mentioned in our meeting today, that only works for file systems that access the tv_sec/tv_nsec members directly, or that use the simple_getattr/generic_fillattr and simple_setattr/setattr_copy helpers to access them.
One counterexample I found is the cifs_set_file_info() function that passes attrs->ia_atime() into another function that takes a timespec argument. There are only a couple of those, but we need to have a strategy for handling them.
It would also be nice to have a generic way to get/set just the seconds portion with a helper, so we don't need to use a timespec64 at all in file systems that simply deal with whole seconds.
I only noticed this problem when I started with ext4. Even though they allow higher granularity, they seem to be treating sec and nsec field as independent values. I need to do a couple of more fs to do this correctly. This is why I picked FAT next.
Ok.
Arnd