On Wed, Jun 22, 2022 at 07:54:33PM +0300, Amir Goldstein wrote:
> On Wed, Jun 22, 2022 at 7:38 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
> >
> > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote:
> > > From: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> > >
> > > commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.
> > >
> > > In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
> > > into xfs_fs_remount. The only time that we ever need to push dirty file
> > > data or metadata to disk for a remount is if we're remounting the
> > > filesystem read only, so this really could be moved to xfs_remount_ro.
> > >
> > > Once we've moved the call site, actually check the return value from
> > > sync_filesystem.
>
> This part is not really relevant for this backport, do you want me to
> emphasise that?
Not relevant? Making sync_fs return error codes to callers was the
entire reason for creating this series...
> > >
> > > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
> > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_super.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 6323974d6b3e..dd0439ae6732 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1716,6 +1716,11 @@ xfs_remount_ro(
> > > };
> > > int error;
> > >
> > > + /* Flush all the dirty data to disk. */
> > > + error = sync_filesystem(mp->m_super);
> >
> > Looking at 5.10.124's fsync.c and xfs_super.c:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/sync.c?h=v5.10.124#n31
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/xfs/xfs_super.c?h=v5.10.124#n755
> >
> > I think this kernel needs the patch(es) that make __sync_filesystem return
> > the errors passed back by ->sync_fs, and I think also the patch that
> > makes xfs_fs_sync_fs return errors encountered by xfs_log_force, right?
>
> It wasn't my intention to fix syncfs() does not return errors in 5.10.
> It has always been that way and IIRC, the relevant patches did not
> apply cleanly.
...because right now userspace can call syncfs() on a filesystem that
dies in the process, and the VFS eats the EIO and returns 0 to
userspace. Yes, that's the historical behavior fo 5.10, but that's a
serious problem that needs addressing. Eliding the sync_filesystem call
during a rw remount is not itself all that exciting.
> THIS patch however, fixes something else, not only the return of the error
> to its caller, so I thought it was worth backporting.
Assuming "something else" means "moving the sync_filesystem callsite" --
that was a secondary piece that I did to get the requisite RVB tag under
time pressure after 5.17-rc6 dropped.
> If you think otherwise, I'll drop it.
On the contrary, I think the ->sync_fs fixes *also* need backporting.
It should be as simple as patching __sync_filesystem:
static int __sync_filesystem(struct super_block *sb, int wait)
{
if (wait)
sync_inodes_sb(sb);
else
writeback_inodes_sb(sb, WB_REASON_SYNC);
if (sb->s_op->sync_fs) {
int ret = sb->s_op->sync_fs(sb, wait);
if (ret)
return ret;
}
return __sync_blockdev(sb->s_bdev, wait);
}
Granted, that can be a part of the next batch. If you plan to pick up
the vfs sync_fs changes then I guess this one's ok for inclusion now.
--D
>
> Thanks,
> Amir.
