Message ID | 20220617100641.1653164-10-amir73il@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xfs stable candidate patches for 5.10.y (v5.15+) | expand |
On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote: > From: "Darrick J. Wong" <djwong@kernel.org> > > 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. > > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Reviewed-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > 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? --D > + if (error) > + return error; > + > /* > * Cancel background eofb scanning so it cannot race with the final > * log force+buftarg wait and deadlock the remount. > @@ -1786,8 +1791,6 @@ xfs_fc_reconfigure( > if (error) > return error; > > - sync_filesystem(mp->m_super); > - > /* inode32 -> inode64 */ > if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && > !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) { > -- > 2.25.1 >
On Wed, Jun 22, 2022 at 7:38 PM Darrick J. Wong <djwong@kernel.org> wrote: > > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote: > > From: "Darrick J. Wong" <djwong@kernel.org> > > > > 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? > > > > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()") > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > 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. THIS patch however, fixes something else, not only the return of the error to its caller, so I thought it was worth backporting. If you think otherwise, I'll drop it. Thanks, Amir.
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@kernel.org> wrote: > > > > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote: > > > From: "Darrick J. Wong" <djwong@kernel.org> > > > > > > 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@kernel.org> > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > 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.
On Thu, Jun 23, 2022 at 2:42 AM Darrick J. Wong <djwong@kernel.org> 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@kernel.org> wrote: > > > > > > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote: > > > > From: "Darrick J. Wong" <djwong@kernel.org> > > > > > > > > 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@kernel.org> > > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > 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.
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); + if (error) + return error; + /* * Cancel background eofb scanning so it cannot race with the final * log force+buftarg wait and deadlock the remount. @@ -1786,8 +1791,6 @@ xfs_fc_reconfigure( if (error) return error; - sync_filesystem(mp->m_super); - /* inode32 -> inode64 */ if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {