On Tue, May 06, 2025 at 04:38:36PM +0200, Christoph Hellwig wrote:
> + rcu_read_lock();
> + page = brd_lookup_page(brd, sector);
> + if (!page && op_is_write(opf)) {
> /*
> * Must use NOIO because we don't want to recurse back into the
> * block or filesystem layers from page reclaim.
> */
> - err = brd_insert_page(brd, sector,
> - (opf & REQ_NOWAIT) ? GFP_NOWAIT : GFP_NOIO);
> - if (err) {
> - if (err == -ENOMEM && (opf & REQ_NOWAIT))
> - bio_wouldblock_error(bio);
> - else
> - bio_io_error(bio);
> - return false;
> + gfp_t gfp = (opf & REQ_NOWAIT) ? GFP_NOWAIT : GFP_NOIO;
> +
> + rcu_read_unlock();
> + page = alloc_page(gfp | __GFP_ZERO | __GFP_HIGHMEM);
> + if (!page) {
> + err = -ENOMEM;
> + goto out_error;
> + }
> + rcu_read_lock();
> +
> + xa_lock(&brd->brd_pages);
> + ret = __xa_store(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT,
> + page, gfp);
On success, __xa_store() says it replaces the old entry ("ret"), with
your new entry ("page"). I think you want to store the new entry only if
there is no old entry, so shouldn't this instead be:
ret = __xa_cmpxchg(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT,
NULL, page, gfp);
?
> + if (!ret)
> + brd->brd_nr_pages++;
> + xa_unlock(&brd->brd_pages);
> +
> + if (ret) {
> + __free_page(page);
> + err = xa_err(ret);
> + if (err < 0)
> + goto out_error;
> + page = ret;
> }
