在 2024/11/9 1:30, Darrick J. Wong 写道:
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. :)
OK, I got it. Thank you for your reply.
Thanks,
Zizhi Wo
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@xxxxxxxxxx>
---
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
