| Message ID | 20210423131050.141140-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: set aside allocation btree blocks from block reservation | expand |
On 23 Apr 2021 at 18:40, Brian Foster wrote: > perag reservation is enabled at mount time on a per AG basis. The > upcoming change to set aside allocbt blocks from block reservation > requires a populated allocbt counter as soon as possible after mount > to be fully effective against large perag reservations. Therefore as > a preparation step, initialize the pagf on all mounts where at least > one reservation is active. Note that this already occurs to some > degree on most default format filesystems as reservation requirement > calculations already depend on the AGF or AGI, depending on the > reservation type. > The changes look good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c > index 6c5f8d10589c..e32a1833d523 100644 > --- a/fs/xfs/libxfs/xfs_ag_resv.c > +++ b/fs/xfs/libxfs/xfs_ag_resv.c > @@ -253,7 +253,8 @@ xfs_ag_resv_init( > xfs_agnumber_t agno = pag->pag_agno; > xfs_extlen_t ask; > xfs_extlen_t used; > - int error = 0; > + int error = 0, error2; > + bool has_resv = false; > > /* Create the metadata reservation. */ > if (pag->pag_meta_resv.ar_asked == 0) { > @@ -291,6 +292,8 @@ xfs_ag_resv_init( > if (error) > goto out; > } > + if (ask) > + has_resv = true; > } > > /* Create the RMAPBT metadata reservation */ > @@ -304,19 +307,28 @@ xfs_ag_resv_init( > error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used); > if (error) > goto out; > + if (ask) > + has_resv = true; > } > > -#ifdef DEBUG > - /* need to read in the AGF for the ASSERT below to work */ > - error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0); > - if (error) > - return error; > - > - ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > - xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > - pag->pagf_freeblks + pag->pagf_flcount); > -#endif > out: > + /* > + * Initialize the pagf if we have at least one active reservation on the > + * AG. This may have occurred already via reservation calculation, but > + * fall back to an explicit init to ensure the in-core allocbt usage > + * counters are initialized as soon as possible. This is important > + * because filesystems with large perag reservations are susceptible to > + * free space reservation problems that the allocbt counter is used to > + * address. > + */ > + if (has_resv) { > + error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0); > + if (error2) > + return error2; > + ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > + xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > + pag->pagf_freeblks + pag->pagf_flcount); > + } > return error; > }
On 4/23/21 6:10 AM, Brian Foster wrote: > perag reservation is enabled at mount time on a per AG basis. The > upcoming change to set aside allocbt blocks from block reservation > requires a populated allocbt counter as soon as possible after mount > to be fully effective against large perag reservations. Therefore as > a preparation step, initialize the pagf on all mounts where at least > one reservation is active. Note that this already occurs to some > degree on most default format filesystems as reservation requirement > calculations already depend on the AGF or AGI, depending on the > reservation type. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks ok to me Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c > index 6c5f8d10589c..e32a1833d523 100644 > --- a/fs/xfs/libxfs/xfs_ag_resv.c > +++ b/fs/xfs/libxfs/xfs_ag_resv.c > @@ -253,7 +253,8 @@ xfs_ag_resv_init( > xfs_agnumber_t agno = pag->pag_agno; > xfs_extlen_t ask; > xfs_extlen_t used; > - int error = 0; > + int error = 0, error2; > + bool has_resv = false; > > /* Create the metadata reservation. */ > if (pag->pag_meta_resv.ar_asked == 0) { > @@ -291,6 +292,8 @@ xfs_ag_resv_init( > if (error) > goto out; > } > + if (ask) > + has_resv = true; > } > > /* Create the RMAPBT metadata reservation */ > @@ -304,19 +307,28 @@ xfs_ag_resv_init( > error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used); > if (error) > goto out; > + if (ask) > + has_resv = true; > } > > -#ifdef DEBUG > - /* need to read in the AGF for the ASSERT below to work */ > - error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0); > - if (error) > - return error; > - > - ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > - xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > - pag->pagf_freeblks + pag->pagf_flcount); > -#endif > out: > + /* > + * Initialize the pagf if we have at least one active reservation on the > + * AG. This may have occurred already via reservation calculation, but > + * fall back to an explicit init to ensure the in-core allocbt usage > + * counters are initialized as soon as possible. This is important > + * because filesystems with large perag reservations are susceptible to > + * free space reservation problems that the allocbt counter is used to > + * address. > + */ > + if (has_resv) { > + error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0); > + if (error2) > + return error2; > + ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > + xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > + pag->pagf_freeblks + pag->pagf_flcount); > + } > return error; > } > >
On Fri, Apr 23, 2021 at 09:10:48AM -0400, Brian Foster wrote: > perag reservation is enabled at mount time on a per AG basis. The > upcoming change to set aside allocbt blocks from block reservation > requires a populated allocbt counter as soon as possible after mount > to be fully effective against large perag reservations. Therefore as > a preparation step, initialize the pagf on all mounts where at least > one reservation is active. Note that this already occurs to some > degree on most default format filesystems as reservation requirement > calculations already depend on the AGF or AGI, depending on the > reservation type. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good to me too, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c > index 6c5f8d10589c..e32a1833d523 100644 > --- a/fs/xfs/libxfs/xfs_ag_resv.c > +++ b/fs/xfs/libxfs/xfs_ag_resv.c > @@ -253,7 +253,8 @@ xfs_ag_resv_init( > xfs_agnumber_t agno = pag->pag_agno; > xfs_extlen_t ask; > xfs_extlen_t used; > - int error = 0; > + int error = 0, error2; > + bool has_resv = false; > > /* Create the metadata reservation. */ > if (pag->pag_meta_resv.ar_asked == 0) { > @@ -291,6 +292,8 @@ xfs_ag_resv_init( > if (error) > goto out; > } > + if (ask) > + has_resv = true; > } > > /* Create the RMAPBT metadata reservation */ > @@ -304,19 +307,28 @@ xfs_ag_resv_init( > error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used); > if (error) > goto out; > + if (ask) > + has_resv = true; > } > > -#ifdef DEBUG > - /* need to read in the AGF for the ASSERT below to work */ > - error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0); > - if (error) > - return error; > - > - ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > - xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > - pag->pagf_freeblks + pag->pagf_flcount); > -#endif > out: > + /* > + * Initialize the pagf if we have at least one active reservation on the > + * AG. This may have occurred already via reservation calculation, but > + * fall back to an explicit init to ensure the in-core allocbt usage > + * counters are initialized as soon as possible. This is important > + * because filesystems with large perag reservations are susceptible to > + * free space reservation problems that the allocbt counter is used to > + * address. > + */ > + if (has_resv) { > + error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0); > + if (error2) > + return error2; > + ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + > + xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= > + pag->pagf_freeblks + pag->pagf_flcount); > + } > return error; > } > > -- > 2.26.3 >
diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c index 6c5f8d10589c..e32a1833d523 100644 --- a/fs/xfs/libxfs/xfs_ag_resv.c +++ b/fs/xfs/libxfs/xfs_ag_resv.c @@ -253,7 +253,8 @@ xfs_ag_resv_init( xfs_agnumber_t agno = pag->pag_agno; xfs_extlen_t ask; xfs_extlen_t used; - int error = 0; + int error = 0, error2; + bool has_resv = false; /* Create the metadata reservation. */ if (pag->pag_meta_resv.ar_asked == 0) { @@ -291,6 +292,8 @@ xfs_ag_resv_init( if (error) goto out; } + if (ask) + has_resv = true; } /* Create the RMAPBT metadata reservation */ @@ -304,19 +307,28 @@ xfs_ag_resv_init( error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used); if (error) goto out; + if (ask) + has_resv = true; } -#ifdef DEBUG - /* need to read in the AGF for the ASSERT below to work */ - error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0); - if (error) - return error; - - ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + - xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= - pag->pagf_freeblks + pag->pagf_flcount); -#endif out: + /* + * Initialize the pagf if we have at least one active reservation on the + * AG. This may have occurred already via reservation calculation, but + * fall back to an explicit init to ensure the in-core allocbt usage + * counters are initialized as soon as possible. This is important + * because filesystems with large perag reservations are susceptible to + * free space reservation problems that the allocbt counter is used to + * address. + */ + if (has_resv) { + error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0); + if (error2) + return error2; + ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + + xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= + pag->pagf_freeblks + pag->pagf_flcount); + } return error; }
perag reservation is enabled at mount time on a per AG basis. The upcoming change to set aside allocbt blocks from block reservation requires a populated allocbt counter as soon as possible after mount to be fully effective against large perag reservations. Therefore as a preparation step, initialize the pagf on all mounts where at least one reservation is active. Note that this already occurs to some degree on most default format filesystems as reservation requirement calculations already depend on the AGF or AGI, depending on the reservation type. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)