Re: [RFC v2a 11/12] net: ceph: use vfs_time data type instead of timespec

[Dave Chinner]

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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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.
-- 
Dave Chinner
david@xxxxxxxxxxxxx