Re: [PATCH 1/2] xfs: Fix missing block calculations in xfs datadev fsmap

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Nov 08, 2024 at 10:29:08AM +0800, Zizhi Wo wrote:
> 
> 
> 在 2024/11/8 7:43, Darrick J. Wong 写道:
> > On Mon, Aug 26, 2024 at 11:10:04AM +0800, Zizhi Wo wrote:
> > > In xfs datadev fsmap query, I noticed a missing block calculation problem:
> > > [root@fedora ~]# xfs_db -r -c "sb 0" -c "p" /dev/vdb
> > > magicnum = 0x58465342
> > > blocksize = 4096
> > > dblocks = 5242880
> > > ......
> > > [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
> > > ...
> > > 30: 253:16 [31457384..41943031]: free space            3  (104..10485751)    10485648
> > > 
> > > (41943031 + 1) / 8 = 5242879 != 5242880
> > > We missed one block in our fsmap calculation!
> > 
> > Eek.
> > 
> > > The root cause of the problem lies in __xfs_getfsmap_datadev(), where the
> > > calculation of "end_fsb" requires a classification discussion. If "end_fsb"
> > > is calculated based on "eofs", we need to add an extra sentinel node for
> > > subsequent length calculations. Otherwise, one block will be missed. If
> > > "end_fsb" is calculated based on "keys[1]", then there is no need to add an
> > > extra node. Because "keys[1]" itself is unreachable, it cancels out one of
> > > the additions. The diagram below illustrates this:
> > > 
> > > |0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|-----eofs
> > > |---------------|---------------------|
> > > a       n       b         n+1         c
> > > 
> > > Assume that eofs is 16, the start address of the previous query is block n,
> > > sector 0, and the length is 1, so the "info->next" is at point b, sector 8.
> > > In the last query, suppose the "rm_startblock" calculated based on
> > > "eofs - 1" is the last block n+1 at point b. All we get is the starting
> > > address of the block, not the end. Therefore, an additional sentinel node
> > > needs to be added to move it to point c. After that, subtracting one from
> > > the other will yield the remaining 1.
> > > 
> > > Although we can now calculate the exact last query using "info->end_daddr",
> > > we will still get an incorrect value if the device at this point is not the
> > > boundary device specified by "keys[1]", as "end_daddr" is still the initial
> > > value. Therefore, the eofs situation here needs to be corrected. The issue
> > > is resolved by adding a sentinel node.
> > 
> > Why don't we set end_daddr unconditionally, then?
> > 
> > Hmm, looking at the end_daddr usage in fsmap.c, I think it's wrong.  If
> > end_daddr is set at all, it's set either to the last sector for which
> > the user wants a mapping; or it's set to the last sector for the device.
> > But then look at how we use it:
> > 
> > 	if (info->last...)
> > 		frec->start_daddr = info->end_daddr;
> > 
> > 	...
> > 
> > 	/* "report the gap..."
> > 	if (frec->start_daddr > info->next_daddr) {
> > 		fmr.fmr_length = frec->start_daddr - info->next_daddr;
> > 	}
> > 
> > This is wrong -- we're using start_daddr to compute the distance from
> > the last mapping that we output up to the end of the range that we want.
> > The "end of the range" is modeled with a phony rmap record that starts
> > at the first fsblock after that range.
> > 
> 
> In the current code, we set "rec_daddr = end_daddr" only when
> (info->last && info->end_daddr != NULL), which should ensure that this
> is the last query?

Right.

> Because end_daddr is set to the last device, and
> info->last is set to the last query. Therefore, assigning it to
> start_daddr should not cause issues in the next query?

Right, the code currently sets end_daddr only for the last device, so
there won't be any issues with the next query.

That said, we reset the xfs_getfsmap_info state between each device, so
it's safe to set end_daddr for every device, not just the last time
through that loop.

> Did I misunderstand something? Or is it because the latest code
> constantly updates end_daddr, which is why this issue arises?

The 6.13 metadir/rtgroups patches didn't change when end_daddr gets set,
but my fixpatch *does* make it set end_daddr for all devices.  Will send
a patch + fstests update shortly to demonstrate. :)

> > IOWs, that assignment should have been
> > frec->start_daddr = info->end_daddr + 1.
> > 
> > Granted in August the codebase was less clear about the difference
> > between rec_daddr and rmap->rm_startblock.  For 6.13, hch cleaned all
> > that up -- rec_daddr is now called start_daddr and the fsmap code passes
> > rmap records with space numbers in units of daddrs via a new struct
> > xfs_fsmap_rec.  Unfortunately, that's all buried in the giant pile of
> > pull requests I sent a couple of days ago which hasn't shown up on
> > for-next yet.
> > 
> > https://lore.kernel.org/linux-xfs/173084396955.1871025.18156568347365549855.stgit@frogsfrogsfrogs/
> > 
> > So I think I know how to fix this against the 6.13 codebase, but I'm
> > going to take a slightly different approach than yours...
> > 
> > > Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
> > > Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> > > ---
> > >   fs/xfs/xfs_fsmap.c | 19 +++++++++++++++++--
> > >   1 file changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > > index 85dbb46452ca..8a2dfe96dae7 100644
> > > --- a/fs/xfs/xfs_fsmap.c
> > > +++ b/fs/xfs/xfs_fsmap.c
> > > @@ -596,12 +596,27 @@ __xfs_getfsmap_datadev(
> > >   	xfs_agnumber_t			end_ag;
> > >   	uint64_t			eofs;
> > >   	int				error = 0;
> > > +	int				sentinel = 0;
> > >   	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
> > >   	if (keys[0].fmr_physical >= eofs)
> > >   		return 0;
> > >   	start_fsb = XFS_DADDR_TO_FSB(mp, keys[0].fmr_physical);
> > > -	end_fsb = XFS_DADDR_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
> > > +	/*
> > > +	 * For the case of eofs, we need to add a sentinel node;
> > > +	 * otherwise, one block will be missed when calculating the length
> > > +	 * in the last query.
> > > +	 * For the case of key[1], there is no need to add a sentinel node
> > > +	 * because it already represents a value that cannot be reached.
> > > +	 * For the case where key[1] after shifting is within the same
> > > +	 * block as the starting address, it is resolved using end_daddr.
> > > +	 */
> > > +	if (keys[1].fmr_physical > eofs - 1) {
> > > +		sentinel = 1;
> > > +		end_fsb = XFS_DADDR_TO_FSB(mp, eofs - 1);
> > > +	} else {
> > > +		end_fsb = XFS_DADDR_TO_FSB(mp, keys[1].fmr_physical);
> > > +	}
> > 
> > ...because running against djwong-wtf, I actually see the same symptoms
> > for the realtime device.  So I think a better solution is to change
> > xfs_getfsmap to set end_daddr always, and then fix the off by one error.
> > 
> 
> Yes, my second patch looks at this rt problem...
> Thank you for your reply

<nod>

--D

> Thanks,
> Zizhi Wo
> 
> 
> > I also don't really like "sentinel" values because they're not
> > intuitive.
> > 
> > I will also go update xfs/273 to check that there are no gaps in the
> > mappings returned, and that they go to where the filesystem thinks is
> > the end of the device.  Thanks for reporting this, sorry I was too busy
> > trying to get metadir/rtgroups done to look at this until now. :(
> > 
> > --D
> > 
> 
> 
> 
> > >   	/*
> > >   	 * Convert the fsmap low/high keys to AG based keys.  Initialize
> > > @@ -649,7 +664,7 @@ __xfs_getfsmap_datadev(
> > >   		info->pag = pag;
> > >   		if (pag->pag_agno == end_ag) {
> > >   			info->high.rm_startblock = XFS_FSB_TO_AGBNO(mp,
> > > -					end_fsb);
> > > +					end_fsb) + sentinel;
> > >   			info->high.rm_offset = XFS_BB_TO_FSBT(mp,
> > >   					keys[1].fmr_offset);
> > >   			error = xfs_fsmap_owner_to_rmap(&info->high, &keys[1]);
> > > -- 
> > > 2.39.2
> > > 
> > > 
>