On Tue, Sep 20, 2022 at 01:56:25PM -0400, Mikulas Patocka wrote:
> This patch extends the rcu regions, so that lookup followed by a read or
> write of a page is done inside rcu read lock. This si be needed for the
> following patch that enables discard.
>
> Note that we also replace "BUG_ON(!page);" with "if (page) ..." in
> copy_to_brd - the page may be NULL if write races with discard. In this
> situation, the result is undefined, so we can actually skip the write
> operation at all.
>
> Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
>
> ---
> drivers/block/brd.c | 59 +++++++++++++++++++++++-----------------------------
> 1 file changed, 27 insertions(+), 32 deletions(-)
>
> Index: linux-2.6/drivers/block/brd.c
> ===================================================================
> --- linux-2.6.orig/drivers/block/brd.c
> +++ linux-2.6/drivers/block/brd.c
> @@ -50,31 +50,12 @@ struct brd_device {
>
> /*
> * Look up and return a brd's page for a given sector.
> + * This must be called with the rcu lock held.
> */
> static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
> {
> + pgoff_t idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
> + return radix_tree_lookup(&brd->brd_pages, idx);
> }
This is still missing the rcu_read_lock_held() assertation if you
want to keep it as separate function.
> + rcu_read_lock();
> + page = brd_lookup_page(brd, sector);
> + if (page) {
> + dst = kmap_atomic(page);
> + memcpy(dst + offset, src, copy);
> + kunmap_atomic(dst);
> + }
> + rcu_read_unlock();
How is the null check going to work here? Simply not copying
data is no exactly the expected result.
This is why I think we need the higher level rework I suggested
last time where we have a helper that always gives you page
(or maybe an error) by moving the insert so that it also does
the actual final lookup.
