Re: [Y2038] [PATCH v3 1/1] generic/402: Make timestamp range check conditional

[Amir Goldstein]

On Sun, Jan 19, 2020 at 2:57 AM Deepa Dinamani  wrote:
>
> Addition of fs-specific timestamp range checking was added
> in 188d20bcd1eb ("vfs: Add file timestamp range support").
>
> Add a check for whether the kernel supports the limits check
> before running the associated test.
>
> Based on an off-list discussion, we use a simpler interim approach
> until fsinfo syscall would provide fs timestamp limits info.
> This isn't perfect, but works for filesystems expiring in 2038.
>
> Suggested-by: Amir Goldstein 
> Signed-off-by: Deepa Dinamani 
> ---

Excellent! Thank you.

Eryu, you may add any of:
Reviewed-by: Amir Goldstein 
Tested-by: Amir Goldstein 

On kernel v5.4, ext2,ext4,xfs,btrfs (default mkfs) still pass.
On Kernel v5.3, ext2,xfs are notrun for lack of kernel support
(instead of failing),
ext4 (256 bytes inodes) still fails
and btrfs still pass, because bash overflows $(($s64max + 1 )) just the same as
the kernel...

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2] generic/402: Make timestamp range check conditional

[Amir Goldstein]

> > I understand why you dislike the ext2+loop test, but please hear me out.
> >
> > From all the fs types that are supported by the test, only btrfs and ext4 
> > with
> > large inode size are y2038 ready.
> > For the rest of the cases, we actually have a way to detect kernel support
> > from the dmesg warning, without the need for hacky ext2 loop mount.
> >
> > So how about just:
> > 1. moving  _scratch_mount before _require_timestamp_range
> > 2. in _require_timestamp_range (untested):
> > if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
>
> Yeah, this looks fine. I thought about searching for dmesg warning after
> _scratch_mount as well, but that would _notrun if the fs is 2038-safe.
> This $tsmax check fixes my concern. Thanks!
>

Deepa,

Do you intend to post the simplified version or would you like me
to re-spin your patch?

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2] generic/402: Make timestamp range check conditional

[Amir Goldstein]

On Wed, Jan 8, 2020 at 10:09 AM Eryu Guan  wrote:
>
> On Mon, Dec 30, 2019 at 09:34:47AM +0200, Amir Goldstein wrote:
> > On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani  
> > wrote:
> > >
> > > Addition of fs-specific timestamp range checking was added
> > > in 188d20bcd1eb ("vfs: Add file timestamp range support").
> > >
> > > Add a check for whether the kernel supports the limits check
> > > before running the associated test.
> > >
> > > ext4 has been chosen to test for the presence of kernel support
> > > for this feature. If there is a concern that ext4 could be built
> > > out of the kernel, I can include a _require_ext4() along the
> > > lines of _require_ext2().
> > >
> > > Suggested-by: Amir Goldstein 
> > > Signed-off-by: Deepa Dinamani 
>
> Sorry for chiming in so late..
>
> > > ---
> > > * Changes since v1:
> > >   used loopback device instead of mkfs scratch dev
> > >
> > >  common/rc | 26 ++
> > >  tests/generic/402 |  3 +++
> > >  2 files changed, 29 insertions(+)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index 816588d6..6248adf7 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1981,6 +1981,32 @@ _run_aiodio()
> > >  return $status
> > >  }
> > >
> > > +_require_kernel_timestamp_range()
> > > +{
> > > +   LOOP_FILE=$SCRATCH_MNT/loop_file
> > > +   LOOP_MNT=$SCRATCH_MNT/loop_mnt
> > > +
> > > +   dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || 
> > > _fail "loopback prep failed"
> > > +
> > > +   # Use ext4 with 128-byte inodes, which do not have room for 
> > > extended timestamp
> > > +   FSTYP=ext4 MKFS_OPTIONS=-I128 \
> > > +   _mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs 
> > > failed"
> > > +
> > > +   LOOP_DEV=$(_create_loop_device $LOOP_FILE)
> > > +   mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to 
> > > create $LOOP_MNT"
> > > +   mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || 
> > > _fail "ext4 mount failed"
> > > +   notrun=false
> > > +   _check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} 
> > > supports timestamps until 2038" || \
> > > +   notrun=true
> > > +
> > > +   umount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount 
> > > $LOOP_MNT"
> > > +   _destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1
> > > +
> > > +   if $notrun; then
> > > +   _notrun "Kernel does not support timestamp limits"
> > > +   fi
> > > +}
> > > +
> >
> > As a generic helper, this function has a few problems:
> > 1. It assumes scratch dev is mounted (and you're not even calling it
> > after _scratch_mount)
> > 2. The cleanup() hook won't clean loop mnt/dev if interrupted
> > 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
> >
> > All this leads me to think that perhaps it would be better off, at least 
> > until
> > kernel has fsinfo, to keep this entire helper inside generic/402,
> > while addressing
> > the issues above in the test itself.
> >
> > A more generic solution would be harder and IMO and overkill at this point.
> >
> > What do you think?
>
> After reading through this thread, I prefer waiting for the comming
> fsinfo interface, detecting the timestamp limit support using ext2 &
> loop device doesn't look "pretty" and is just a temporary solution.
>

I understand why you dislike the ext2+loop test, but please hear me out.

From all the fs types that are supported by the test, only btrfs and ext4 with
large inode size are y2038 ready.
For the rest of the cases, we actually have a way to detect kernel support
from the dmesg warning, without the need for hacky ext2 loop mount.

So how about just:
1. moving  _scratch_mount before _require_timestamp_range
2. in _require_timestamp_range (untested):
if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
timestamps until 2038" ; then
_notrun "Kernel does not support timestamp limits"
fi

It's better than nothing and it does not add much complications, nor
is this "hacky"
IMO.

Thoughts?

Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2] generic/402: Make timestamp range check conditional

[Amir Goldstein]

On Fri, Jan 3, 2020 at 8:46 AM Deepa Dinamani  wrote:
>
> > As a generic helper, this function has a few problems:
> > 1. It assumes scratch dev is mounted (and you're not even calling it
> > after _scratch_mount)
>
> Ok, this was an oversight. I was using rootfs as scratch and didn't
> catch this in my tests.
>
> > 2. The cleanup() hook won't clean loop mnt/dev if interrupted
>
> Agreed. I can add these in.
>
> There are some existing functions in common/rc that do not clean up.
> For example _require_scratch_swapfile might leave partial state if 
> interrupted.

Right. Things are not perfect.
We should try to not make them worse ;-)

>
> > 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
>
> Some generic functions assume many preconditions.
> But, if it is preferred to be more self contained, I can model this
> after something like _require_scratch_swapfile()

OK.

>
> > All this leads me to think that perhaps it would be better off, at least 
> > until
> > kernel has fsinfo, to keep this entire helper inside generic/402,
> > while addressing
> > the issues above in the test itself.
>
> > A more generic solution would be harder and IMO and overkill at this point.
>
> With fsinfo as proposed, it would not be possible to tell if fs ranges
> are supported without doing the same kind of workaround.
> A capability bit could be added to advertise that feature of VFS, or
> it might be reasonable to assume it from the mere existence of fsinfo
> syscall.

That is what I had in mind.

>
> > What do you think?
>
> The following proposed patch uses a local _cleanup handler that can handle 
> this.
> I am okay with either approach. I'm not sure which one is preferable
> to xfstests maintainers.
> Let me know and I can post it as a V3.
>

IMO the "nested" _cleanup handler is not a "clean" solution,
but let's let Eryu decide how to tackle this.

Thanks,
Amir.

>
> From f539005511f3ad83563cabc6d185b2c76ae37dea Mon Sep 17 00:00:00 2001
> From: Deepa Dinamani 
> Date: Sun, 22 Dec 2019 19:18:14 -0800
> Subject: [PATCH v3 1/1] generic/402: Make timestamp range check conditional
>
> Addition of fs-specific timestamp range checking was added
> in 188d20bcd1eb ("vfs: Add file timestamp range support").
>
> Add a check for whether the kernel supports the limits check
> before running the associated test.
>
> ext4 has been chosen to test for the presence of kernel support
> for this feature.
>
> Suggested-by: Amir Goldstein 
> Signed-off-by: Deepa Dinamani 
> ---
>  common/rc | 50 +++
>  tests/generic/402 | 12 +++-
>  2 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index eeac1355..796052e6 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1751,17 +1751,18 @@ _require_loop()
>  #
>  _require_ext2()
>  {
> +fs=${1:-ext2}
>  if [ "$HOSTOS" != "Linux" ]
>  then
> -_notrun "This test requires linux for ext2 filesystem support"
> +_notrun "This test requires linux for $fs filesystem support"
>  fi
>
> -modprobe ext2 >/dev/null 2>&1
> -if grep ext2 /proc/filesystems >/dev/null 2>&1
> +modprobe $fs >/dev/null 2>&1
> +if grep $fs /proc/filesystems >/dev/null 2>&1
>  then
>  :
>  else
> -_notrun "This test requires ext2 filesystem support"
> +_notrun "This test requires $fs filesystem support"
>  fi
>  }
>
> @@ -1981,6 +1982,47 @@ _run_aiodio()
>  return $status
>  }
>
> +_require_kernel_timestamp_range()
> +{
> +
> +_require_scratch
> +_require_loop
> +_require_ext2 ext4
> +
> +# Use a subshell to clean up the nested loop mount
> +_cleanup='( umount $LOOP_MNT ; _destroy_loop_device $LOOP_DEV ;
> rm -f $LOOP_FILE ; _scratch_unmount )'
> +
> +_scratch_mkfs >/dev/null
> +_scratch_mount
> +
> +LOOP_FILE=$SCRATCH_MNT/loop_file
> +LOOP_MNT=$SCRATCH_MNT/loop_mnt
> +
> +dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd ||
> _fail "loopback prep failed"
> +
> +# Use ext4 with 128-byte inodes, which do not have room for
> extended timestamp
> +FSTYP=ext4 MKFS_OPTIONS=-I128 \
> +_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"
> +
> +LOOP_DEV=$(_create_loop_device $LOOP_FILE)
> +mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to

Re: [Y2038] [PATCH v2] generic/402: Make timestamp range check conditional

[Amir Goldstein]

On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani  wrote:
>
> Addition of fs-specific timestamp range checking was added
> in 188d20bcd1eb ("vfs: Add file timestamp range support").
>
> Add a check for whether the kernel supports the limits check
> before running the associated test.
>
> ext4 has been chosen to test for the presence of kernel support
> for this feature. If there is a concern that ext4 could be built
> out of the kernel, I can include a _require_ext4() along the
> lines of _require_ext2().
>
> Suggested-by: Amir Goldstein 
> Signed-off-by: Deepa Dinamani 
> ---
> * Changes since v1:
>   used loopback device instead of mkfs scratch dev
>
>  common/rc | 26 ++
>  tests/generic/402 |  3 +++
>  2 files changed, 29 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 816588d6..6248adf7 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1981,6 +1981,32 @@ _run_aiodio()
>  return $status
>  }
>
> +_require_kernel_timestamp_range()
> +{
> +   LOOP_FILE=$SCRATCH_MNT/loop_file
> +   LOOP_MNT=$SCRATCH_MNT/loop_mnt
> +
> +   dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || 
> _fail "loopback prep failed"
> +
> +   # Use ext4 with 128-byte inodes, which do not have room for extended 
> timestamp
> +   FSTYP=ext4 MKFS_OPTIONS=-I128 \
> +   _mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"
> +
> +   LOOP_DEV=$(_create_loop_device $LOOP_FILE)
> +   mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create 
> $LOOP_MNT"
> +   mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail 
> "ext4 mount failed"
> +   notrun=false
> +   _check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} 
> supports timestamps until 2038" || \
> +   notrun=true
> +
> +   umount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount 
> $LOOP_MNT"
> +   _destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1
> +
> +   if $notrun; then
> +   _notrun "Kernel does not support timestamp limits"
> +   fi
> +}
> +

As a generic helper, this function has a few problems:
1. It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount)
2. The cleanup() hook won't clean loop mnt/dev if interrupted
3. test doesn't have _require_loop (nor require ext4 as you mentioned)

All this leads me to think that perhaps it would be better off, at least until
kernel has fsinfo, to keep this entire helper inside generic/402,
while addressing
the issues above in the test itself.

A more generic solution would be harder and IMO and overkill at this point.

What do you think?

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] generic/402: Make timestamp range check conditional

[Amir Goldstein]

On Mon, Dec 23, 2019 at 7:16 AM Deepa Dinamani  wrote:
>
> Addition of fs-specific timestamp range checking was added
> in 188d20bcd1eb ("vfs: Add file timestamp range support").
>
> Add a check for whether the kernel supports the limits check
> before running the associated test.
>
> Signed-off-by: Deepa Dinamani 
> ---
>  common/rc | 11 +++
>  tests/generic/402 |  3 +++
>  2 files changed, 14 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 816588d6..472db995 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1981,6 +1981,17 @@ _run_aiodio()
>  return $status
>  }
>
> +_require_kernel_timestamp_range()
> +{
> +   # 128-byte inodes do not have room for extended timestamp
> +   MKFS_OPTIONS=-I128 _scratch_mkfs_ext4 &>> $seqres.full 2>&1 || _fail 
> "ext4 mkfs failed"
> +
> +   mount -t ext4 ${SCRATCH_DEV} ${SCRATCH_MNT}
> +   _check_dmesg_for "ext4 filesystem being mounted at ${SCRATCH_MNT} 
> supports timestamps until 2038" || \
> +   _notrun "Kernel does not support timestamp limits"
> +   umount ${SCRATCH_MNT}
> +}
> +

Deepa,

Thank you for following up.
I am not sure if mkfs.ext4 of scratch partition in a generic test is going to be
very popular - let's see what others have to say.
You can certainly now do that without checking that  ${SCRATCH_DEV} is
a blockdev which is not the case for overlay and networking filesystems.

Why did you choose not to use a loop mounted ext2 for the check as
I suggested?
You can use _require_loop() and _require_ext2() inside the check.
In any case, please also check for failure to mount.

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] generic/402: fix for updated behavior of timestamp limits

[Amir Goldstein]

On Thu, Dec 19, 2019 at 10:28 AM Arnd Bergmann  wrote:
>
> On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein  wrote:
> >
> > I don't think there is a clear policy about being friendly to testing
> > less that master kernels in xfstest (Eryu?), but IMO we should try to
> > accommodate
> > this use case, because it is in the best interest of everyone that stable 
> > kernel
> > will be regularly tested with xfstests with as little noisy failures
> > as possible.
>
> I think what makes this one particularly hard is that there are most likely
> people that do care about the failure on older kernels being reported and
> would rather backport the kernel changes into their product kernels
> to have them behave sanely.

Getting back to the thread before it diverged into the backport option.

The test used to detect kernel support and be skipped automatically on
old kernel
and now the test fails because the kernel knob and test for it were removed.

I perceive that as a regression to the test.
I don't mind waiting for fsinfo() to fix this regression, as long as
fsinfo() is going to
be backported to stable kernel 5.4???

Deepa,

You added this warning:
pr_warn("Mounted %s file system at %s supports timestamps until ...
along with timestamp clamping

I suggest that you implement kernel support check based on grepping
for this warning after loop mounting an ext2 test image.
A bit over the top and ugly, but the test should be reliable and
mkfs.ext2 is probably available in all xfstest deployments.

xfs/049 makes use of an ext2 loop mount, so your test won't be the
first one to use ext2 for testing other fs.

When kernel gets fsinfo() the test for kernel support can be improved
to using fsinfo() before doing the ext2 loop mount hack.

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] generic/402: fix for updated behavior of timestamp limits

[Amir Goldstein]

On Wed, Dec 18, 2019 at 10:21 PM Deepa Dinamani  wrote:
>
> I looked at this more closely. Here is the patch that added the sysctl
> to the kernel previously: https://lkml.org/lkml/2016/11/2/300.
>
> This was meant to be configurable earlier. That is why this made
> sense. But, now it is not. We unconditionally clamp to the fs limits.
> I looked around to see if we ever expose information about internal
> kernel changes to userspace. This is almost never done. And, this is
> always in the form of maybe a syscall failing. Given that we don't see
> any modified behaviour that the user can point out, I don't think we
> can expose the presence of clamping in the kernel.
>
> fsinfo though exposes a fs max and min that could be useful if we fill
> in an unknown pattern as default:
> https://lwn.net/ml/linux-api/153314004147.18964.11925284995448007945.st...@warthog.procyon.org.uk/.
>
> I also spoke about this to Arnd, and he also suggested the fsinfo as
> an alternative.
>
> Is it easy to not run the test on older kernels? Otherwise, we just
> have to rely on fsinfo being merged?
>

Is it easy to blacklist the test if that is what you are asking.
How people run their stable kernel tests I don't know.
I believe Sasha is running xfstests to validate stable release
candidate patches.

I don't think there is a clear policy about being friendly to testing
less that master kernels in xfstest (Eryu?), but IMO we should try to
accommodate
this use case, because it is in the best interest of everyone that stable kernel
will be regularly tested with xfstests with as little noisy failures
as possible.

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] generic/402: fix for updated behavior of timestamp limits

[Amir Goldstein]

On Fri, Jul 19, 2019 at 7:21 AM Deepa Dinamani  wrote:
>
> The mount behavior will not be altered because of the unsupported
> timestamps on the filesystems.
>
> Adjust the test accordingly.
>
> An updated series to be posted after the merge window is hosted at
> 
>
> Signed-off-by: Deepa Dinamani 
> ---
>  common/rc | 36 +-
>  tests/generic/402 | 87 ---
>  tests/generic/402.out |  2 +-
>  3 files changed, 53 insertions(+), 72 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 25203bb4..39a2deb0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1959,16 +1959,9 @@ _run_aiodio()
>  return $status
>  }
>
> -# this test requires y2038 sysfs switch and filesystem
> -# timestamp ranges support.
> -_require_y2038()
> +_require_timestamp_range()
>  {
> local device=${1:-$TEST_DEV}
> -   local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
> -
> -   if [ ! -e $sysfsdir ]; then
> -   _notrun "no kernel support for y2038 sysfs switch"
> -   fi
>

Deepa,

This change, which is already merged removed the test for kernel support
and replaced it with test only for filesystem support.

This impacts people validating stable kernel releases with xfstest, because
this test now always fails on stable kernels and I don't think that timestamp
clamping behavior is going to stable kernels.

Of course stable kernel testers can exclude this test, but this will remove test
coverage and may result in silent breakage of > y2038 timetamps in stable
kernels.

I think test should identify if kernel had clamping behavior and change the
expected timestamp values accordingly, similar to how the test was before
adjusting it to new behavior.

Do you agree? Can you make these changes?

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

On Mon, Nov 25, 2019 at 6:46 PM J . Bruce Fields  wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> So, nfsd has always bypassed timestamp_truncate() and we've never
> noticed till now?  What are the symptoms?  (Do timestamps go backwards
> after cache eviction on filesystems with large time granularity?)

Clamping seems to be new behavior since v5.4-rc1.
Before that clamping was done implicitly when hitting the disk IIUC,
so it was observed mostly after cache eviction.

>
> Looks like generic/402 has never run in my tests:
>
> generic/402 [not run] no kernel support for y2038 sysfs switch
>

The test in its current form is quite recent as well or at the _require
has changed recently.
See acb2ba78 - overlay: support timestamp range check

You'd probably need something similar for nfs (?)

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

On Sun, Nov 24, 2019 at 9:49 PM Al Viro  wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> Makes sense; said that, shouldn't we go through ->setattr() instances and
> get rid of that there, now that notify_change() is made to do it?
>

Sounds reasonable. But I'd rather leave this cleanup to Deepa,
who did all this work.

Thanks,
Amir.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

[Y2038] [PATCH] utimes: Clamp the timestamps in notify_change()

[Amir Goldstein]

Push clamping timestamps down the call stack into notify_change(), so
in-kernel callers like nfsd and overlayfs will get similar timestamp
set behavior as utimes.

Suggested-by: Miklos Szeredi 
Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
Cc: sta...@vger.kernel.org # v5.4
Cc: Deepa Dinamani 
Cc: Jeff Layton 
Signed-off-by: Amir Goldstein 
---

Arnd,

This fixes xfstest generic/402 when run with -overlay setup.
Note that running the test requires latest xfstests with:
 acb2ba78 - overlay: support timestamp range check

I had previously posted a fix specific for overlayfs [1],
but Miklos suggested this more generic fix, which should also
serve nfsd and other in-kernel users.

I tested this change with test generic/402 on ext4/xfs/btrfs
and overlayfs, but not with nfsd.

Jeff, could you ack this change is good for nfsd as well?

Thanks,
Amir.

[1] 
https://lore.kernel.org/linux-fsdevel/2019073000.2957-1-amir7...@gmail.com/

 fs/attr.c   | 5 +
 fs/utimes.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index df28035aa23e..e8de5e636e66 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * 
attr, struct inode **de
attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
attr->ia_atime = now;
+   else
+   attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
+   else
+   attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+
if (ia_valid & ATTR_KILL_PRIV) {
error = security_inode_need_killpriv(dentry);
if (error < 0)
diff --git a/fs/utimes.c b/fs/utimes.c
index 1ba3f7883870..090739322463 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct 
timespec64 *times)
if (times[0].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_ATIME;
else if (times[0].tv_nsec != UTIME_NOW) {
-   newattrs.ia_atime = timestamp_truncate(times[0], inode);
+   newattrs.ia_atime = times[0];
newattrs.ia_valid |= ATTR_ATIME_SET;
}
 
if (times[1].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_MTIME;
else if (times[1].tv_nsec != UTIME_NOW) {
-   newattrs.ia_mtime = timestamp_truncate(times[1], inode);
+   newattrs.ia_mtime = times[1];
newattrs.ia_valid |= ATTR_MTIME_SET;
}
/*
-- 
2.17.1

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RFC 4/5] xfs: extend inode format for 40-bit timestamps

[Amir Goldstein]

On Tue, Nov 12, 2019 at 4:16 PM Christoph Hellwig  wrote:
>
> Amir just send another patch dealing with the time stamps.  I'd suggest
> you chime into the discussion in that thread.

That's right I just posted the ext4 style extend to 34bits yesterday [1],
but I like your version so much better, so I will withdraw mine.

Sorry I did not CC you nor Deepa nor y2038 list.
I did not think you were going to actually deal with specific filesystems.

I'd also like to hear people's thoughts about migration process.
Should the new feature be ro_compat as I defined it or incompat?

If all agree that Arnd's format change is preferred, I can assist with
xfsprogs patches, tests or whatnot.

Thanks,
Amir.

[1] 
https://lore.kernel.org/linux-xfs/20191112082524.ga18...@infradead.org/T/#mfa11ea3c035d4c21ec6a56b7c83a6dfa76e48068
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038