Re: [PATCH v2] ovl: don't set the superblock's errseq_t manually

Re: [PATCH v2] ovl: don't set the superblock's errseq_t manually

[Amir Goldstein]

On Tue, Jul 30, 2024 at 6:20 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>
> Since commit 5679897eb104 ("vfs: make sync_filesystem return errors from
> ->sync_fs"), the return value from sync_fs callback can be seen in
> sync_filesystem(). Thus the errseq_set opreation can be removed here.
>
> Depends-on: commit 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> Changes since v1:
> - Add Depends-on and Reviewed-by tags.
> ---
>  fs/overlayfs/super.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 06a231970cb5..fe511192f83c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -202,15 +202,9 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
>         int ret;
>
>         ret = ovl_sync_status(ofs);
> -       /*
> -        * We have to always set the err, because the return value isn't
> -        * checked in syncfs, and instead indirectly return an error via
> -        * the sb's writeback errseq, which VFS inspects after this call.
> -        */
> -       if (ret < 0) {
> -               errseq_set(&sb->s_wb_err, -EIO);
> +
> +       if (ret < 0)
>                 return -EIO;
> -       }
>
>         if (!ret)
>                 return ret;
> --
> 2.25.1
>

FYI, this change is queued in overlayfs-next.

However, I went to see if overlayfs has test coverage for this and it does not.

The test coverage added by Darrick to the mentioned vfs commit is test xfs/546,
so it does not run on other fs, although it is quite generic.

I fixed this test so it could run on overlayfs (like this):
# This command is complicated a bit because in the case of overlayfs the
# syncfs fd needs to be opened before shutdown and it is different from the
# shutdown fd, so we cannot use the _scratch_shutdown() helper.
# Filter out xfs_io output of active fds.
$XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
' -c close -c syncfs $SCRATCH_MNT | \
        grep -vF '[00'

and it passes on both xfs and overlayfs (over xfs), but if I try to
make it "generic"
it fails on ext4, which explicitly allows syncfs after shutdown:

        if (unlikely(ext4_forced_shutdown(sb)))
                return 0;

Ted, Darrick,

Do you have any insight as to why this ext4 behavior differs from xfs
or another idea how to exercise the syncfs error in a generic test?

I could fork an overlay/* test from the xfs/* test and require that
underlying fs is xfs, but that would be ugly.

Any ideas?

Thanks,
Amir.

Re: [PATCH v2] ovl: don't set the superblock's errseq_t manually

[Darrick J. Wong]

On Fri, Aug 30, 2024 at 03:27:35PM +0200, Amir Goldstein wrote:
> On Tue, Jul 30, 2024 at 6:20 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
> >
> > Since commit 5679897eb104 ("vfs: make sync_filesystem return errors from
> > ->sync_fs"), the return value from sync_fs callback can be seen in
> > sync_filesystem(). Thus the errseq_set opreation can be removed here.
> >
> > Depends-on: commit 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> > Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > Changes since v1:
> > - Add Depends-on and Reviewed-by tags.
> > ---
> >  fs/overlayfs/super.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 06a231970cb5..fe511192f83c 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -202,15 +202,9 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
> >         int ret;
> >
> >         ret = ovl_sync_status(ofs);
> > -       /*
> > -        * We have to always set the err, because the return value isn't
> > -        * checked in syncfs, and instead indirectly return an error via
> > -        * the sb's writeback errseq, which VFS inspects after this call.
> > -        */
> > -       if (ret < 0) {
> > -               errseq_set(&sb->s_wb_err, -EIO);
> > +
> > +       if (ret < 0)
> >                 return -EIO;
> > -       }
> >
> >         if (!ret)
> >                 return ret;
> > --
> > 2.25.1
> >
> 
> FYI, this change is queued in overlayfs-next.
> 
> However, I went to see if overlayfs has test coverage for this and it does not.
> 
> The test coverage added by Darrick to the mentioned vfs commit is test xfs/546,
> so it does not run on other fs, although it is quite generic.
> 
> I fixed this test so it could run on overlayfs (like this):
> # This command is complicated a bit because in the case of overlayfs the
> # syncfs fd needs to be opened before shutdown and it is different from the
> # shutdown fd, so we cannot use the _scratch_shutdown() helper.
> # Filter out xfs_io output of active fds.
> $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
> ' -c close -c syncfs $SCRATCH_MNT | \
>         grep -vF '[00'
> 
> and it passes on both xfs and overlayfs (over xfs), but if I try to
> make it "generic"
> it fails on ext4, which explicitly allows syncfs after shutdown:
> 
>         if (unlikely(ext4_forced_shutdown(sb)))
>                 return 0;
> 
> Ted, Darrick,
> 
> Do you have any insight as to why this ext4 behavior differs from xfs
> or another idea how to exercise the syncfs error in a generic test?
> 
> I could fork an overlay/* test from the xfs/* test and require that
> underlying fs is xfs, but that would be ugly.
> 
> Any ideas?

That should be:

	if (unlikely(ext4_forced_shutdown(sb)))
		return -EIO;

no?  The fs is dead and cannot persist anything, so we should fling that
back to the calling program.

--D

> Thanks,
> Amir.
> 

Re: [PATCH v2] ovl: don't set the superblock's errseq_t manually

[Haifeng Xu]

On 2024/8/30 23:26, Darrick J. Wong wrote:
> On Fri, Aug 30, 2024 at 03:27:35PM +0200, Amir Goldstein wrote:
>> On Tue, Jul 30, 2024 at 6:20 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>>>
>>> Since commit 5679897eb104 ("vfs: make sync_filesystem return errors from
>>> ->sync_fs"), the return value from sync_fs callback can be seen in
>>> sync_filesystem(). Thus the errseq_set opreation can be removed here.
>>>
>>> Depends-on: commit 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
>>> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
>>> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>> Changes since v1:
>>> - Add Depends-on and Reviewed-by tags.
>>> ---
>>>  fs/overlayfs/super.c | 10 ++--------
>>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>> index 06a231970cb5..fe511192f83c 100644
>>> --- a/fs/overlayfs/super.c
>>> +++ b/fs/overlayfs/super.c
>>> @@ -202,15 +202,9 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
>>>         int ret;
>>>
>>>         ret = ovl_sync_status(ofs);
>>> -       /*
>>> -        * We have to always set the err, because the return value isn't
>>> -        * checked in syncfs, and instead indirectly return an error via
>>> -        * the sb's writeback errseq, which VFS inspects after this call.
>>> -        */
>>> -       if (ret < 0) {
>>> -               errseq_set(&sb->s_wb_err, -EIO);
>>> +
>>> +       if (ret < 0)
>>>                 return -EIO;
>>> -       }
>>>
>>>         if (!ret)
>>>                 return ret;
>>> --
>>> 2.25.1
>>>
>>
>> FYI, this change is queued in overlayfs-next.
>>
>> However, I went to see if overlayfs has test coverage for this and it does not.
>>
>> The test coverage added by Darrick to the mentioned vfs commit is test xfs/546,
>> so it does not run on other fs, although it is quite generic.
>>
>> I fixed this test so it could run on overlayfs (like this):
>> # This command is complicated a bit because in the case of overlayfs the
>> # syncfs fd needs to be opened before shutdown and it is different from the
>> # shutdown fd, so we cannot use the _scratch_shutdown() helper.
>> # Filter out xfs_io output of active fds.
>> $XFS_IO_PROG -x -c "open $(_scratch_shutdown_handle)" -c 'shutdown -f
>> ' -c close -c syncfs $SCRATCH_MNT | \
>>         grep -vF '[00'
>>
>> and it passes on both xfs and overlayfs (over xfs), but if I try to
>> make it "generic"
>> it fails on ext4, which explicitly allows syncfs after shutdown:
>>
>>         if (unlikely(ext4_forced_shutdown(sb)))
>>                 return 0;
>>
>> Ted, Darrick,
>>
>> Do you have any insight as to why this ext4 behavior differs from xfs
>> or another idea how to exercise the syncfs error in a generic test?
>>
>> I could fork an overlay/* test from the xfs/* test and require that
>> underlying fs is xfs, but that would be ugly.
>>
>> Any ideas?
> 
> That should be:
> 
> 	if (unlikely(ext4_forced_shutdown(sb)))
> 		return -EIO;
> 
> no?  The fs is dead and cannot persist anything, so we should fling that
> back to the calling program.

yes. sync_filesystem() write outs and wait upon all dirty data. If the superblock is shutdown,
writeback can't make any progress and there is no guarantees are made on how many dirty data
have been written out. So I think it's reasonable to tell users about this error.

> 
> --D
> 
>> Thanks,
>> Amir.
>>