On Tue, Jul 12, 2022 at 01:57:59PM +0100, Matthew Wilcox wrote:
> > So I assumed the nodes stored in the xas object, which is local to the
> > collapse_file() function.
>
> Yes, that's a reasonable thing to think, but it's actually not how
> it works. When we allocate a node in xas_create(), we put it straight
> into the tree without storing it in xas->xa_alloc. We may then end
> up not using it, but the node isn't leaked because it's in the tree.
>
> If the GFP_NOWAIT allocation fails (it didn't in these stack traces),
> we call xas_nomem(), which sees an -ENOMEM, allocates a node and stores
> it in xas->xa_alloc; then we go round the loop again where xas_create()
> will take the node from xas->xa_alloc. But the backtraces here don't
> implicate xas_nomem().
There is actually a leak here, but it's not the one that's been found.
do {
xas_lock_irq(&xas);
xas_create_range(&xas);
if (!xas_error(&xas))
break;
xas_unlock_irq(&xas);
if (!xas_nomem(&xas, GFP_KERNEL)) {
result = SCAN_FAIL;
goto out;
}
} while (1);
If xas_create() fails, it sets xas error to -ENOMEM. So we unlock the
xarray lock and call xas_nomem(). xas_nomem() sees the error is -ENOMEM
and allocates a node with GFP_KERNEL, putting the node in xas->xa_alloc.
We re-take the spinlock and call xas_create() again. If we still need
a node, xas_alloc() will take the node stored in xas->xa_alloc. If we
raced and don't need a node, xas_create_range() succeeds and we break out,
failing to free the xas->xa_alloc node.
We should call xas_destroy() at the out: label. However, this does not
explain what is going on, and will not fix what is going on, since any
nodes allocated during xas_create_range() will be stored safely in the
tree and will not be freed by xas_destroy().
