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:
So this makes a lot of sense now. If it's the only use of timespec64_to_inode_time/inode_time_to_timespec64, you could also open-code that in the macro and avoid introducing the macro to start with.
This is how I had it to begin with. But, I think having this in separate function makes the code more modular. Are we trying to minimize number of functions?
Generally the optimization should prefer readability and performance. I was thinking here that the code is easier to follow if there is only one intermediate step that we introduce rather than two.
Another option would be to drop the structure entirely and put the members directly into the inode as i_atime_sec/i_atime_nsec/i_mtime_sec/ etc. That would also avoid marking the 64-bit members as 'packed', which may be inefficient on some CPU architectures.
I considered this option. This would need weird way of adding the fields: iatime_sec; imtime_sec; ictime_sec; iatime_nsec; iatime_nsec; ictime_nsec;
Yes, this is what I had in mind. In David Howells' xstat syscall series, they are done the same way, including in user space. The main advantage is that all fields are naturally aligned, so they take a couple of cycles less to access than a misaligned 64-bit field on certain architectures.
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?
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 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.
With your approach, another advantage is how any code that assigns i_?time to current_fs_time() will not have to be modified, while separate fields do need the modification.
There is also some code already in timespec_trunc that limits the precision to the resolution supported by the file system in the setattr functions. We can extend timespec_trunc to also limit the range, as long as it is used consistently.
Ok. Yes, this seems like a good place to do the range checking. I will keep this in mind when I post an update.
Here is my plan:
1.Update the patches according to above comments. 2.Global vfs substitutions with inline functions. 3. Will extend ext4 and fat to support these inline functions as well. 4. Will use these file systems to do some testing. 5. Send out all the above patches for review together.
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).
I'm modifying them concurrently and do not have a separate step. I could post the vfs patch as its done except for the one function I mentioned above.
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.
-Deepa