On Sat, Feb 13, 2016 at 05:46:11PM -0800, Deepa Dinamani wrote:
On Sat, Feb 13, 2016 at 2:08 PM, Dave Chinner david@fromorbit.com wrote:
On Fri, Feb 12, 2016 at 01:36:05AM -0800, Deepa Dinamani wrote:
The VFS inode timestamps are not y2038 safe as they use struct timespec. These will be changed to use struct timespec64 instead and that is y2038 safe. But, since the above data type conversion will break the end file systems, use vfs_time aliases here to access inode times.
These timestamps are passed in as arguments to functions using inode timestamps. Hence, these need to change along with vfs to support 64 bit timestamps. vfs_time helps do this transition.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Just a point to highlight the problem with this approach:
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index f8f2359..1273db6 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -2401,7 +2401,7 @@ bad: */ void ceph_osdc_build_request(struct ceph_osd_request *req, u64 off, struct ceph_snap_context *snapc, u64 snap_id,
struct timespec *mtime)
struct vfs_time *mtime){ struct ceph_msg *msg = req->r_request; void *p;
So this change assumes that mtime is not passed by reference to another function. If we change vfs_time to be a timespec64, then dereferencing in this function works fine, but passing to another function will not because that function will be expecting a timespec.
That, indeed, is what happens here. A few lines into this function:
if (req->r_flags & CEPH_OSD_FLAG_WRITE) ceph_encode_timespec(p, mtime);And that function:
static inline void ceph_encode_timespec(struct ceph_timespec *tv, const struct timespec *ts) { tv->tv_sec = cpu_to_le32((u32)ts->tv_sec); tv->tv_nsec = cpu_to_le32((u32)ts->tv_nsec); }
I'm not sure where you picked up this encode function from.
You might be missing the patches( 9 and 10) before this?:
2b5f8e517c6fd7121fc1b890c51c6256bc21beb6 net: ceph: use vfs_time data type instead of timespec ca5b82952a6a522ae058ccede57ba1a71da498c5 fs: ceph: Replace timespec data type with vfs_time 3a3ac0bdd23284c4f27a7ab1c133056c1a998075 fs: ceph: Change encode and decode functions to use vfs_time
So I missed this last patch when quickly reading throught he series. I think this is being broken up way too much, and that makes it hard to see how the changes relate.
i.e. this series could have easily been just 3 or 4 patches - it's only ~100 lines of code total that is changed by the series.
FWIW, let me put this in context for you, so maybe you'll understand why I think this timespec64 changeover is actually a trivial, simply thing. Last week I reviewed about 10,000 lines of new code amongst some 14,000 lines of change (twice) for xfstests. That was amongst getting ~1000 lines of my own changes reviewed and committed into the XFS kernel tree, handling user problems, reviewing and commenting on DAX changes, etc.
This week I only have a libxfs kernel/userspace code sync to do (only 500 lines of change this time), and then I've about 10,000 lines of complex new XFS kernel code to review (i.e. for reverse mapping, reflink, dedupe and copy-on-write support). I'll have to review that more than once, and once that is done and all th changes have been propagated over into the userspace code, I've got another 10,000 lines of code in userspace (again for reflink, etc) to review, test and merge.
So, excuse me if I made a mistake and missed something in a patchset that a) had 3 different versions posted, b) is way too fine-grained, and c) being treated like a mountain when it's really a tiny molehill. I do have much more important things to do with my time than be dragged into another silly "oh this is so difficult and hard" bikeshedding argument when I could easily write the entire patchset to do a timespec64 changeover for the VFS in a couple of hours. It's just not that hard to do.
And, FWIW, I'm still waiting to hear how we're going to regression test all this. Has anyone written any xfstests yet to ensure that all the filesystems behave the same and we won't break anything in future as we add 64 bit timestamp support to filesystem on-disk formats? IMO, there's more work in writing the regression tests to make sure everything works correctly in all the different possible combinations of filesystem, kernel and userspace support (e.g. 32 on 32, 32 on 64, 64 on 32 and 64 on 64). I'm much more concerned about this aspect of the problem than actually changing the VFS code, because without it we can't verify the changes we are making are behaving correctly...
Cheers,
Dave.