| Message ID | 166795953926.3761353.16847285987138337778.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: random fixes for 6.0 | expand |
> Fix this by retaining a reference to the superblock buffer when possible > so that the writeback hook doesn't have to access the buffer cache to > set NEEDSREPAIR. This is the same one you sent for 5.19 that opened a discussion between retaining it at specific points, or at 'mount' time, wasn't it? Anyway, I think this is ok, we can try to do it at mount time in the future. > > Fixes: 3b7667cb ("xfs_repair: set NEEDSREPAIR the first time we write to a filesystem") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > libxfs/libxfs_api_defs.h | 2 + > libxfs/libxfs_io.h | 1 + > libxfs/rdwr.c | 8 +++++ > repair/phase2.c | 8 +++++ > repair/protos.h | 1 + > repair/xfs_repair.c | 75 ++++++++++++++++++++++++++++++++++++++++------ > 6 files changed, 86 insertions(+), 9 deletions(-) > > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index 2716a731bf..f8efcce777 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -53,9 +53,11 @@ > #define xfs_buf_delwri_submit libxfs_buf_delwri_submit > #define xfs_buf_get libxfs_buf_get > #define xfs_buf_get_uncached libxfs_buf_get_uncached > +#define xfs_buf_lock libxfs_buf_lock > #define xfs_buf_read libxfs_buf_read > #define xfs_buf_read_uncached libxfs_buf_read_uncached > #define xfs_buf_relse libxfs_buf_relse > +#define xfs_buf_unlock libxfs_buf_unlock > #define xfs_bunmapi libxfs_bunmapi > #define xfs_bwrite libxfs_bwrite > #define xfs_calc_dquots_per_chunk libxfs_calc_dquots_per_chunk > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h > index 9c0e2704d1..fae8642720 100644 > --- a/libxfs/libxfs_io.h > +++ b/libxfs/libxfs_io.h > @@ -226,6 +226,7 @@ xfs_buf_hold(struct xfs_buf *bp) > } > > void xfs_buf_lock(struct xfs_buf *bp); > +void xfs_buf_unlock(struct xfs_buf *bp); > > int libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen, int flags, > struct xfs_buf **bpp); > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c > index 20e0793c2f..d5aad3ea21 100644 > --- a/libxfs/rdwr.c > +++ b/libxfs/rdwr.c > @@ -384,6 +384,14 @@ xfs_buf_lock( > pthread_mutex_lock(&bp->b_lock); > } > > +void > +xfs_buf_unlock( > + struct xfs_buf *bp) > +{ > + if (use_xfs_buf_lock) > + pthread_mutex_unlock(&bp->b_lock); > +} > + > static int > __cache_lookup( > struct xfs_bufkey *key, > diff --git a/repair/phase2.c b/repair/phase2.c > index 56a39bb456..2ada95aefd 100644 > --- a/repair/phase2.c > +++ b/repair/phase2.c > @@ -370,6 +370,14 @@ phase2( > } else > do_log(_("Phase 2 - using internal log\n")); > > + /* > + * Now that we've set up the buffer cache the way we want it, try to > + * grab our own reference to the primary sb so that the hooks will not > + * have to call out to the buffer cache. > + */ > + if (mp->m_buf_writeback_fn) > + retain_primary_sb(mp); > + > /* Zero log if applicable */ > do_log(_(" - zero log...\n")); > > diff --git a/repair/protos.h b/repair/protos.h > index 03ebae1413..83e471ff2a 100644 > --- a/repair/protos.h > +++ b/repair/protos.h > @@ -16,6 +16,7 @@ int get_sb(xfs_sb_t *sbp, > xfs_off_t off, > int size, > xfs_agnumber_t agno); > +int retain_primary_sb(struct xfs_mount *mp); > void write_primary_sb(xfs_sb_t *sbp, > int size); > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > index 871b428d7d..ff29bea974 100644 > --- a/repair/xfs_repair.c > +++ b/repair/xfs_repair.c > @@ -749,6 +749,63 @@ check_fs_vs_host_sectsize( > } > } > > +/* > + * If we set up a writeback function to set NEEDSREPAIR while the filesystem is > + * dirty, there's a chance that calling libxfs_getsb could deadlock the buffer > + * cache while trying to get the primary sb buffer if the first non-sb write to > + * the filesystem is the result of a cache shake. Retain a reference to the > + * primary sb buffer to avoid all that. > + */ > +static struct xfs_buf *primary_sb_bp; /* buffer for superblock */ > + > +int > +retain_primary_sb( > + struct xfs_mount *mp) > +{ > + int error; > + > + error = -libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR, > + XFS_FSS_TO_BB(mp, 1), 0, &primary_sb_bp, > + &xfs_sb_buf_ops); > + if (error) > + return error; > + > + libxfs_buf_unlock(primary_sb_bp); > + return 0; > +} > + > +static void > +drop_primary_sb(void) > +{ > + if (!primary_sb_bp) > + return; > + > + libxfs_buf_lock(primary_sb_bp); > + libxfs_buf_relse(primary_sb_bp); > + primary_sb_bp = NULL; > +} > + > +static int > +get_primary_sb( > + struct xfs_mount *mp, > + struct xfs_buf **bpp) > +{ > + int error; > + > + *bpp = NULL; > + > + if (!primary_sb_bp) { > + error = retain_primary_sb(mp); > + if (error) > + return error; > + } > + > + libxfs_buf_lock(primary_sb_bp); > + xfs_buf_hold(primary_sb_bp); > + *bpp = primary_sb_bp; > + return 0; > +} > + > /* Clear needsrepair after a successful repair run. */ > static void > clear_needsrepair( > @@ -769,15 +826,14 @@ clear_needsrepair( > do_warn( > _("Cannot clear needsrepair due to flush failure, err=%d.\n"), > error); > - return; > + goto drop; > } > > /* Clear needsrepair from the superblock. */ > - bp = libxfs_getsb(mp); > - if (!bp || bp->b_error) { > + error = get_primary_sb(mp, &bp); > + if (error) { > do_warn( > - _("Cannot clear needsrepair from primary super, err=%d.\n"), > - bp ? bp->b_error : ENOMEM); > + _("Cannot clear needsrepair from primary super, err=%d.\n"), error); > } else { > mp->m_sb.sb_features_incompat &= > ~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; > @@ -786,6 +842,8 @@ clear_needsrepair( > } > if (bp) > libxfs_buf_relse(bp); > +drop: > + drop_primary_sb(); > } > > static void > @@ -808,11 +866,10 @@ force_needsrepair( > xfs_sb_version_needsrepair(&mp->m_sb)) > return; > > - bp = libxfs_getsb(mp); > - if (!bp || bp->b_error) { > + error = get_primary_sb(mp, &bp); > + if (error) { > do_log( > - _("couldn't get superblock to set needsrepair, err=%d\n"), > - bp ? bp->b_error : ENOMEM); > + _("couldn't get superblock to set needsrepair, err=%d\n"), error); > } else { > /* > * It's possible that we need to set NEEDSREPAIR before we've >
On Fri, Nov 18, 2022 at 03:45:03PM +0100, Carlos Maiolino wrote: > > Fix this by retaining a reference to the superblock buffer when possible > > so that the writeback hook doesn't have to access the buffer cache to > > set NEEDSREPAIR. > > This is the same one you sent for 5.19 that opened a discussion between > retaining it at specific points, or at 'mount' time, wasn't it? Anyway, I think > this is ok, we can try to do it at mount time in the future. Correct, it hasn't changed since then. --D > > > > > Fixes: 3b7667cb ("xfs_repair: set NEEDSREPAIR the first time we write to a filesystem") > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > libxfs/libxfs_api_defs.h | 2 + > > libxfs/libxfs_io.h | 1 + > > libxfs/rdwr.c | 8 +++++ > > repair/phase2.c | 8 +++++ > > repair/protos.h | 1 + > > repair/xfs_repair.c | 75 ++++++++++++++++++++++++++++++++++++++++------ > > 6 files changed, 86 insertions(+), 9 deletions(-) > > > > > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > > index 2716a731bf..f8efcce777 100644 > > --- a/libxfs/libxfs_api_defs.h > > +++ b/libxfs/libxfs_api_defs.h > > @@ -53,9 +53,11 @@ > > #define xfs_buf_delwri_submit libxfs_buf_delwri_submit > > #define xfs_buf_get libxfs_buf_get > > #define xfs_buf_get_uncached libxfs_buf_get_uncached > > +#define xfs_buf_lock libxfs_buf_lock > > #define xfs_buf_read libxfs_buf_read > > #define xfs_buf_read_uncached libxfs_buf_read_uncached > > #define xfs_buf_relse libxfs_buf_relse > > +#define xfs_buf_unlock libxfs_buf_unlock > > #define xfs_bunmapi libxfs_bunmapi > > #define xfs_bwrite libxfs_bwrite > > #define xfs_calc_dquots_per_chunk libxfs_calc_dquots_per_chunk > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h > > index 9c0e2704d1..fae8642720 100644 > > --- a/libxfs/libxfs_io.h > > +++ b/libxfs/libxfs_io.h > > @@ -226,6 +226,7 @@ xfs_buf_hold(struct xfs_buf *bp) > > } > > > > void xfs_buf_lock(struct xfs_buf *bp); > > +void xfs_buf_unlock(struct xfs_buf *bp); > > > > int libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen, int flags, > > struct xfs_buf **bpp); > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c > > index 20e0793c2f..d5aad3ea21 100644 > > --- a/libxfs/rdwr.c > > +++ b/libxfs/rdwr.c > > @@ -384,6 +384,14 @@ xfs_buf_lock( > > pthread_mutex_lock(&bp->b_lock); > > } > > > > +void > > +xfs_buf_unlock( > > + struct xfs_buf *bp) > > +{ > > + if (use_xfs_buf_lock) > > + pthread_mutex_unlock(&bp->b_lock); > > +} > > + > > static int > > __cache_lookup( > > struct xfs_bufkey *key, > > diff --git a/repair/phase2.c b/repair/phase2.c > > index 56a39bb456..2ada95aefd 100644 > > --- a/repair/phase2.c > > +++ b/repair/phase2.c > > @@ -370,6 +370,14 @@ phase2( > > } else > > do_log(_("Phase 2 - using internal log\n")); > > > > + /* > > + * Now that we've set up the buffer cache the way we want it, try to > > + * grab our own reference to the primary sb so that the hooks will not > > + * have to call out to the buffer cache. > > + */ > > + if (mp->m_buf_writeback_fn) > > + retain_primary_sb(mp); > > + > > /* Zero log if applicable */ > > do_log(_(" - zero log...\n")); > > > > diff --git a/repair/protos.h b/repair/protos.h > > index 03ebae1413..83e471ff2a 100644 > > --- a/repair/protos.h > > +++ b/repair/protos.h > > @@ -16,6 +16,7 @@ int get_sb(xfs_sb_t *sbp, > > xfs_off_t off, > > int size, > > xfs_agnumber_t agno); > > +int retain_primary_sb(struct xfs_mount *mp); > > void write_primary_sb(xfs_sb_t *sbp, > > int size); > > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > index 871b428d7d..ff29bea974 100644 > > --- a/repair/xfs_repair.c > > +++ b/repair/xfs_repair.c > > @@ -749,6 +749,63 @@ check_fs_vs_host_sectsize( > > } > > } > > > > +/* > > + * If we set up a writeback function to set NEEDSREPAIR while the filesystem is > > + * dirty, there's a chance that calling libxfs_getsb could deadlock the buffer > > + * cache while trying to get the primary sb buffer if the first non-sb write to > > + * the filesystem is the result of a cache shake. Retain a reference to the > > + * primary sb buffer to avoid all that. > > + */ > > +static struct xfs_buf *primary_sb_bp; /* buffer for superblock */ > > + > > +int > > +retain_primary_sb( > > + struct xfs_mount *mp) > > +{ > > + int error; > > + > > + error = -libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR, > > + XFS_FSS_TO_BB(mp, 1), 0, &primary_sb_bp, > > + &xfs_sb_buf_ops); > > + if (error) > > + return error; > > + > > + libxfs_buf_unlock(primary_sb_bp); > > + return 0; > > +} > > + > > +static void > > +drop_primary_sb(void) > > +{ > > + if (!primary_sb_bp) > > + return; > > + > > + libxfs_buf_lock(primary_sb_bp); > > + libxfs_buf_relse(primary_sb_bp); > > + primary_sb_bp = NULL; > > +} > > + > > +static int > > +get_primary_sb( > > + struct xfs_mount *mp, > > + struct xfs_buf **bpp) > > +{ > > + int error; > > + > > + *bpp = NULL; > > + > > + if (!primary_sb_bp) { > > + error = retain_primary_sb(mp); > > + if (error) > > + return error; > > + } > > + > > + libxfs_buf_lock(primary_sb_bp); > > + xfs_buf_hold(primary_sb_bp); > > + *bpp = primary_sb_bp; > > + return 0; > > +} > > + > > /* Clear needsrepair after a successful repair run. */ > > static void > > clear_needsrepair( > > @@ -769,15 +826,14 @@ clear_needsrepair( > > do_warn( > > _("Cannot clear needsrepair due to flush failure, err=%d.\n"), > > error); > > - return; > > + goto drop; > > } > > > > /* Clear needsrepair from the superblock. */ > > - bp = libxfs_getsb(mp); > > - if (!bp || bp->b_error) { > > + error = get_primary_sb(mp, &bp); > > + if (error) { > > do_warn( > > - _("Cannot clear needsrepair from primary super, err=%d.\n"), > > - bp ? bp->b_error : ENOMEM); > > + _("Cannot clear needsrepair from primary super, err=%d.\n"), error); > > } else { > > mp->m_sb.sb_features_incompat &= > > ~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; > > @@ -786,6 +842,8 @@ clear_needsrepair( > > } > > if (bp) > > libxfs_buf_relse(bp); > > +drop: > > + drop_primary_sb(); > > } > > > > static void > > @@ -808,11 +866,10 @@ force_needsrepair( > > xfs_sb_version_needsrepair(&mp->m_sb)) > > return; > > > > - bp = libxfs_getsb(mp); > > - if (!bp || bp->b_error) { > > + error = get_primary_sb(mp, &bp); > > + if (error) { > > do_log( > > - _("couldn't get superblock to set needsrepair, err=%d\n"), > > - bp ? bp->b_error : ENOMEM); > > + _("couldn't get superblock to set needsrepair, err=%d\n"), error); > > } else { > > /* > > * It's possible that we need to set NEEDSREPAIR before we've > > > > -- > Carlos Maiolino
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h index 2716a731bf..f8efcce777 100644 --- a/libxfs/libxfs_api_defs.h +++ b/libxfs/libxfs_api_defs.h @@ -53,9 +53,11 @@ #define xfs_buf_delwri_submit libxfs_buf_delwri_submit #define xfs_buf_get libxfs_buf_get #define xfs_buf_get_uncached libxfs_buf_get_uncached +#define xfs_buf_lock libxfs_buf_lock #define xfs_buf_read libxfs_buf_read #define xfs_buf_read_uncached libxfs_buf_read_uncached #define xfs_buf_relse libxfs_buf_relse +#define xfs_buf_unlock libxfs_buf_unlock #define xfs_bunmapi libxfs_bunmapi #define xfs_bwrite libxfs_bwrite #define xfs_calc_dquots_per_chunk libxfs_calc_dquots_per_chunk diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h index 9c0e2704d1..fae8642720 100644 --- a/libxfs/libxfs_io.h +++ b/libxfs/libxfs_io.h @@ -226,6 +226,7 @@ xfs_buf_hold(struct xfs_buf *bp) } void xfs_buf_lock(struct xfs_buf *bp); +void xfs_buf_unlock(struct xfs_buf *bp); int libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen, int flags, struct xfs_buf **bpp); diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c index 20e0793c2f..d5aad3ea21 100644 --- a/libxfs/rdwr.c +++ b/libxfs/rdwr.c @@ -384,6 +384,14 @@ xfs_buf_lock( pthread_mutex_lock(&bp->b_lock); } +void +xfs_buf_unlock( + struct xfs_buf *bp) +{ + if (use_xfs_buf_lock) + pthread_mutex_unlock(&bp->b_lock); +} + static int __cache_lookup( struct xfs_bufkey *key, diff --git a/repair/phase2.c b/repair/phase2.c index 56a39bb456..2ada95aefd 100644 --- a/repair/phase2.c +++ b/repair/phase2.c @@ -370,6 +370,14 @@ phase2( } else do_log(_("Phase 2 - using internal log\n")); + /* + * Now that we've set up the buffer cache the way we want it, try to + * grab our own reference to the primary sb so that the hooks will not + * have to call out to the buffer cache. + */ + if (mp->m_buf_writeback_fn) + retain_primary_sb(mp); + /* Zero log if applicable */ do_log(_(" - zero log...\n")); diff --git a/repair/protos.h b/repair/protos.h index 03ebae1413..83e471ff2a 100644 --- a/repair/protos.h +++ b/repair/protos.h @@ -16,6 +16,7 @@ int get_sb(xfs_sb_t *sbp, xfs_off_t off, int size, xfs_agnumber_t agno); +int retain_primary_sb(struct xfs_mount *mp); void write_primary_sb(xfs_sb_t *sbp, int size); diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index 871b428d7d..ff29bea974 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -749,6 +749,63 @@ check_fs_vs_host_sectsize( } } +/* + * If we set up a writeback function to set NEEDSREPAIR while the filesystem is + * dirty, there's a chance that calling libxfs_getsb could deadlock the buffer + * cache while trying to get the primary sb buffer if the first non-sb write to + * the filesystem is the result of a cache shake. Retain a reference to the + * primary sb buffer to avoid all that. + */ +static struct xfs_buf *primary_sb_bp; /* buffer for superblock */ + +int +retain_primary_sb( + struct xfs_mount *mp) +{ + int error; + + error = -libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR, + XFS_FSS_TO_BB(mp, 1), 0, &primary_sb_bp, + &xfs_sb_buf_ops); + if (error) + return error; + + libxfs_buf_unlock(primary_sb_bp); + return 0; +} + +static void +drop_primary_sb(void) +{ + if (!primary_sb_bp) + return; + + libxfs_buf_lock(primary_sb_bp); + libxfs_buf_relse(primary_sb_bp); + primary_sb_bp = NULL; +} + +static int +get_primary_sb( + struct xfs_mount *mp, + struct xfs_buf **bpp) +{ + int error; + + *bpp = NULL; + + if (!primary_sb_bp) { + error = retain_primary_sb(mp); + if (error) + return error; + } + + libxfs_buf_lock(primary_sb_bp); + xfs_buf_hold(primary_sb_bp); + *bpp = primary_sb_bp; + return 0; +} + /* Clear needsrepair after a successful repair run. */ static void clear_needsrepair( @@ -769,15 +826,14 @@ clear_needsrepair( do_warn( _("Cannot clear needsrepair due to flush failure, err=%d.\n"), error); - return; + goto drop; } /* Clear needsrepair from the superblock. */ - bp = libxfs_getsb(mp); - if (!bp || bp->b_error) { + error = get_primary_sb(mp, &bp); + if (error) { do_warn( - _("Cannot clear needsrepair from primary super, err=%d.\n"), - bp ? bp->b_error : ENOMEM); + _("Cannot clear needsrepair from primary super, err=%d.\n"), error); } else { mp->m_sb.sb_features_incompat &= ~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; @@ -786,6 +842,8 @@ clear_needsrepair( } if (bp) libxfs_buf_relse(bp); +drop: + drop_primary_sb(); } static void @@ -808,11 +866,10 @@ force_needsrepair( xfs_sb_version_needsrepair(&mp->m_sb)) return; - bp = libxfs_getsb(mp); - if (!bp || bp->b_error) { + error = get_primary_sb(mp, &bp); + if (error) { do_log( - _("couldn't get superblock to set needsrepair, err=%d\n"), - bp ? bp->b_error : ENOMEM); + _("couldn't get superblock to set needsrepair, err=%d\n"), error); } else { /* * It's possible that we need to set NEEDSREPAIR before we've