On Fri, Aug 30, 2019 at 4:02 AM Deepa Dinamani <[email protected]> wrote:
> On Thu, Aug 29, 2019 at 6:20 PM Mike Marshall <[email protected]> wrote:
> >
> > Hi Deepa...
> >
> > I installed this patch series on top of Linux 5.3-rc6 and ran xfstests
> > on orangefs and got a regression... generic/258 failed
> > with: "Timestamp wrapped"...
> >
> > # cat results/generic/258.out.bad
> > QA output created by 258
> > Creating file with timestamp of Jan 1, 1960
> > Testing for negative seconds since epoch
> > Timestamp wrapped: 0
> > Timestamp wrapped
> > (see /home/hubcap/xfstests-dev/results//generic/258.full for details)
>
> Note that patch [16/20] https://lkml.org/lkml/2019/8/18/193 assumes
> that orangefs does not support negative timestamps.
> And, the reason was pointed out in the commit text:
>
> ----------------------
> Assume the limits as unsigned according to the below
> commit 98e8eef557a9 ("changed PVFS_time from int64_t to uint64_t")
> in https://github.com/waltligon/orangefs
>
> Author: Neill Miller <[email protected]>
> Date: Thu Sep 2 15:00:38 2004 +0000
> --------------------
>
> So the timestamp being wrapped to 0 in this case is correct behavior
> according to my patchset.
> The generic/258 assumes that the timestamps can be negative. If this
> is not true then it should not be run for this fs.
>
> But, if you think the timestamp should support negative timestamps for
> orangefs, I'd be happy to change it.
I think it's unclear from the orangefs source code what the intention is,
as there is a mixed of signed and unsigned types used for the inode
stamps:
#define encode_PVFS_time encode_int64_t
#define encode_int64_t(pptr,x) do { \
*(int64_t*) *(pptr) = cpu_to_le64(*(x)); \
*(pptr) += 8; \
} while (0)
#define decode_PVFS_time decode_int64_t
#define decode_int64_t(pptr,x) do { \
*(x) = le64_to_cpu(*(int64_t*) *(pptr)); \
*(pptr) += 8; \
} while (0)
This suggests that making it unsigned may have been an accident.
Then again, it's clearly and consistently printed as unsigned in
user space:
gossip_debug(
GOSSIP_GETATTR_DEBUG, " VERSION is %llu, mtime is %llu\n",
llu(s_op->attr.mtime), llu(resp_attr->mtime));
A related issue I noticed is this:
PVFS_time PINT_util_mktime_version(PVFS_time time)
{
struct timeval t = {0,0};
PVFS_time version = (time << 32);
gettimeofday(&t, NULL);
version |= (PVFS_time)t.tv_usec;
return version;
}
PVFS_time PINT_util_mkversion_time(PVFS_time version)
{
return (PVFS_time)(version >> 32);
}
static PINT_sm_action getattr_verify_attribs(
struct PINT_smcb *smcb, job_status_s *js_p)
{
...
resp_attr->mtime = PINT_util_mkversion_time(s_op->attr.mtime);
...
}
which suggests that at least for some purposes, the mtime field
is only an unsigned 32-bit number (1970..2106). From my readiing,
this affects the on-disk format, but not the protocol implemented
by the kernel.
atime and ctime are apparently 64-bit, but mtime is only 32-bit
seconds, plus a 32-bit 'version'. I suppose the server could be
fixed to allow a larger range, but probably would take it out of
the 'version' bits, not the upper half.
To be on the safe side, I suppose the kernel can only assume
an unsigned 32-bit range to be available. If the server gets
extended beyond that, it would have to pass a feature flag.
Arnd
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
