On Thu, Jun 23, 2022 at 2:42 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> 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.
Yes, this is a just cause.
>
> > 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.
Right. looking back at my notes:
https://github.com/amir73il/b4/commit/fddd6d961c029441d2f0e9dd86d0f83b754d0c47
I didn't pick this patch at all because of the missing dependencies.
I must have picked it up later from Laeh's series, because
I thought it was harmless to apply the "something else".
>
> > 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.
I picked up the vfs fixes, it was pretty simple as you say.
I even backported "fs: remove __sync_filesystem" for dependency
to make the backport more straightforward.
But that adds 3 more more patches to the series and needs to go
back to testing, so I will just drop patch 9 from this batch.
Thanks,
Amir.
