Re: [PATCH] xfs: fix extent busy updating

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

On Wed, Jan 04, 2023 at 05:41:15PM +0000, Wengang Wang wrote:
> Darrick,
> 
> > On Jan 4, 2023, at 8:24 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> > 
> > On Tue, Jan 03, 2023 at 11:32:17AM -0800, Wengang Wang wrote:
> >> In xfs_extent_busy_update_extent() case 6 and 7, whenever bno is modified on
> >> extent busy, the relavent length has to be modified accordingly.
> >> 
> >> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >> ---
> >> fs/xfs/xfs_extent_busy.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> >> index ad22a003f959..f3d328e4a440 100644
> >> --- a/fs/xfs/xfs_extent_busy.c
> >> +++ b/fs/xfs/xfs_extent_busy.c
> >> @@ -236,6 +236,7 @@ xfs_extent_busy_update_extent(
> >> 		 *
> >> 		 */
> >> 		busyp->bno = fend;
> >> +		busyp->length = bend - fend;
> > 
> > Looks correct to me, but how did you find this?
> 
> I was working with a UEK5 XFS bug where busy blocks (contained in
> extent busy) are allocated to regular files unexpectedly.  When I was
> trying to fix that problem (still reuse busy blocks for directories),
> the problem here is exposed.
> 
> 
> >  Is there some sort of
> > test case we could attach to this?
> 
> Hm.. I can only reproduce this with my patch. Well, the idea is that,
> for example,
> 
> 1) we have an extent busy in the busy tree:    (bno=100, len=200)
> 2) allocate blocks for directories from above extent busy (multiple times)
> 3) after the allocations, above extent busy finally becomes  (bno=300, len=200)  though it should become (bno=300, len=0) and should be removed from the busy tree.
> 4) the block 300 (in that AG) is used as metadata (directory blocks containing dir entries) and then that block is freed
> 5) insert the new extent busy (bno=300, len=1) to the busy tree,
> in function xfs_extent_busy_insert():
> 
>  61         while (*rbp) {
>  62                 parent = *rbp;
>  63                 busyp = rb_entry(parent, struct xfs_extent_busy, rb_node);
>  64
>  65                 if (new->bno < busyp->bno) {
>  66                         rbp = &(*rbp)->rb_left;
>  67                         ASSERT(new->bno + new->length <= busyp->bno);
>  68                 } else if (new->bno > busyp->bno) {
>  69                         rbp = &(*rbp)->rb_right;
>  70                         ASSERT(bno >= busyp->bno + busyp->length);
>  71                 } else {
>  72                         ASSERT(0);
>  73                 }
>  74         }
> 
> Note that node (bno=300, len=200) already exists in the tree, the code
> hits line 72, the “else” case, and enters infinite loop.

Hm.  I /think/ it would be possible but fairly difficult to write an
fstest -- you'd have to create a storage with a very slow DISCARD
command, mount the fs with -o discard, fill the fs, free all the blocks,
make a large directory structure, and then rmdir the directory.  Those
last three steps would have to happen before the discards finish.

Uh... do you think that's worth the effort?  I don't think the kernel
even has a dm driver to simulate a device with only slow discards.

In the meantime,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> thanks,
> wengang
> 
> > 
> > --D
> > 
> >> 	} else if (bbno < fbno) {
> >> 		/*
> >> 		 * Case 8:
> >> -- 
> >> 2.21.0 (Apple Git-122.2)
> >> 
>