| Message ID | 162872992772.1220643.10308054638747493338.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: fix various bugs in fsmap | expand |
On Wed, Aug 11, 2021 at 05:58:47PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > The fsmap implementation for realtime devices uses the gap between > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range > to feed userspace fsmap records with an "unknown" owner. We use this > trick to report to userspace when the last rtextent in the filesystem is > in use by synthesizing a null rmap record starting at the next block > after the query range. > > Unfortunately, there's a minor accounting bug in the way that we > construct the null rmap record. Originally, ahigh.ar_startext contains > the last rtextent for which the user wants records. It's entirely > possible that number is beyond the end of the rt volume, so the location > synthesized rmap record /must/ be constrained to the minimum of the high > key and the number of extents in the rt volume. Looks good, although the change is a little hard to follow due to the big amount of cleanups vs the tiny actual bug fix. Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Aug 12, 2021 at 09:36:52AM +0100, Christoph Hellwig wrote: > On Wed, Aug 11, 2021 at 05:58:47PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > The fsmap implementation for realtime devices uses the gap between > > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range > > to feed userspace fsmap records with an "unknown" owner. We use this > > trick to report to userspace when the last rtextent in the filesystem is > > in use by synthesizing a null rmap record starting at the next block > > after the query range. > > > > Unfortunately, there's a minor accounting bug in the way that we > > construct the null rmap record. Originally, ahigh.ar_startext contains > > the last rtextent for which the user wants records. It's entirely > > possible that number is beyond the end of the rt volume, so the location > > synthesized rmap record /must/ be constrained to the minimum of the high > > key and the number of extents in the rt volume. > > Looks good, although the change is a little hard to follow due to the > big amount of cleanups vs the tiny actual bug fix. I'll clean that up a bit before I commit this to the -merge branch. --D > Reviewed-by: Christoph Hellwig <hch@lst.de>
On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote: > From: Darrick J. Wong <djwong@kernel.org> > > The fsmap implementation for realtime devices uses the gap between > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range > to feed userspace fsmap records with an "unknown" owner. We use this > trick to report to userspace when the last rtextent in the filesystem is > in use by synthesizing a null rmap record starting at the next block > after the query range. > > Unfortunately, there's a minor accounting bug in the way that we > construct the null rmap record. Originally, ahigh.ar_startext contains > the last rtextent for which the user wants records. It's entirely > possible that number is beyond the end of the rt volume, so the location > synthesized rmap record /must/ be constrained to the minimum of the high > key and the number of extents in the rt volume. > When the number of blocks on the realtime device is not an integral multiple of xfs_sb->sb_rextsize, ahigh.ar_startext can contain a value which is one more than the highest valid rtextent. Hence, without this patch, the last record reported to the userpace might contain an invalid upper bound. Assuming that my understanding is indeed correct, Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/xfs_fsmap.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > index 7d0b09c1366e..a0e8ab58124b 100644 > --- a/fs/xfs/xfs_fsmap.c > +++ b/fs/xfs/xfs_fsmap.c > @@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query( > { > struct xfs_rtalloc_rec alow = { 0 }; > struct xfs_rtalloc_rec ahigh = { 0 }; > + struct xfs_mount *mp = tp->t_mountp; > int error; > > - xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); > + xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED); > > + /* > + * Set up query parameters to return free extents covering the range we > + * want. > + */ > alow.ar_startext = info->low.rm_startblock; > + do_div(alow.ar_startext, mp->m_sb.sb_rextsize); > + > ahigh.ar_startext = info->high.rm_startblock; > - do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize); > - if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize)) > + if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize)) > ahigh.ar_startext++; > + > error = xfs_rtalloc_query_range(tp, &alow, &ahigh, > xfs_getfsmap_rtdev_rtbitmap_helper, info); > if (error) > goto err; > > - /* Report any gaps at the end of the rtbitmap */ > + /* > + * Report any gaps at the end of the rtbitmap by simulating a null > + * rmap starting at the block after the end of the query range. > + */ > info->last = true; > + ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext); > + > error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info); > if (error) > goto err; > err: > - xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); > + xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED); > return error; > } >
On Fri, Aug 13, 2021 at 04:20:34PM +0530, Chandan Babu R wrote: > On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > The fsmap implementation for realtime devices uses the gap between > > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range > > to feed userspace fsmap records with an "unknown" owner. We use this > > trick to report to userspace when the last rtextent in the filesystem is > > in use by synthesizing a null rmap record starting at the next block > > after the query range. > > > > Unfortunately, there's a minor accounting bug in the way that we > > construct the null rmap record. Originally, ahigh.ar_startext contains > > the last rtextent for which the user wants records. It's entirely > > possible that number is beyond the end of the rt volume, so the location > > synthesized rmap record /must/ be constrained to the minimum of the high > > key and the number of extents in the rt volume. > > > > When the number of blocks on the realtime device is not an integral multiple > of xfs_sb->sb_rextsize, ahigh.ar_startext can contain a value which is one > more than the highest valid rtextent. Hence, without this patch, the last > record reported to the userpace might contain an invalid upper bound. Assuming > that my understanding is indeed correct, Correct. Thanks for reviewing this! --D > > Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > fs/xfs/xfs_fsmap.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > > index 7d0b09c1366e..a0e8ab58124b 100644 > > --- a/fs/xfs/xfs_fsmap.c > > +++ b/fs/xfs/xfs_fsmap.c > > @@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query( > > { > > struct xfs_rtalloc_rec alow = { 0 }; > > struct xfs_rtalloc_rec ahigh = { 0 }; > > + struct xfs_mount *mp = tp->t_mountp; > > int error; > > > > - xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); > > + xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED); > > > > + /* > > + * Set up query parameters to return free extents covering the range we > > + * want. > > + */ > > alow.ar_startext = info->low.rm_startblock; > > + do_div(alow.ar_startext, mp->m_sb.sb_rextsize); > > + > > ahigh.ar_startext = info->high.rm_startblock; > > - do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize); > > - if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize)) > > + if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize)) > > ahigh.ar_startext++; > > + > > error = xfs_rtalloc_query_range(tp, &alow, &ahigh, > > xfs_getfsmap_rtdev_rtbitmap_helper, info); > > if (error) > > goto err; > > > > - /* Report any gaps at the end of the rtbitmap */ > > + /* > > + * Report any gaps at the end of the rtbitmap by simulating a null > > + * rmap starting at the block after the end of the query range. > > + */ > > info->last = true; > > + ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext); > > + > > error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info); > > if (error) > > goto err; > > err: > > - xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); > > + xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED); > > return error; > > } > > > > > -- > chandan
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c index 7d0b09c1366e..a0e8ab58124b 100644 --- a/fs/xfs/xfs_fsmap.c +++ b/fs/xfs/xfs_fsmap.c @@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query( { struct xfs_rtalloc_rec alow = { 0 }; struct xfs_rtalloc_rec ahigh = { 0 }; + struct xfs_mount *mp = tp->t_mountp; int error; - xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); + xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED); + /* + * Set up query parameters to return free extents covering the range we + * want. + */ alow.ar_startext = info->low.rm_startblock; + do_div(alow.ar_startext, mp->m_sb.sb_rextsize); + ahigh.ar_startext = info->high.rm_startblock; - do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize); - if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize)) + if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize)) ahigh.ar_startext++; + error = xfs_rtalloc_query_range(tp, &alow, &ahigh, xfs_getfsmap_rtdev_rtbitmap_helper, info); if (error) goto err; - /* Report any gaps at the end of the rtbitmap */ + /* + * Report any gaps at the end of the rtbitmap by simulating a null + * rmap starting at the block after the end of the query range. + */ info->last = true; + ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext); + error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info); if (error) goto err; err: - xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED); + xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED); return error; }