| Message ID | 20240315181212.2573753-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | brd: Remove use of page->index | expand |
On Fri, Mar 15, 2024 at 06:12:09PM +0000, Matthew Wilcox (Oracle) wrote: > static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > { > - pgoff_t idx; > - struct page *page; > - > - idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */ > - page = xa_load(&brd->brd_pages, idx); > - > - BUG_ON(page && page->index != idx); > - > - return page; > + return xa_load(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT); > } > > /* > @@ -67,8 +56,8 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > */ > static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) > { > - pgoff_t idx; > - struct page *page, *cur; > + pgoff_t idx = sector >> PAGE_SECTORS_SHIFT; > + struct page *page; > int ret = 0; > > page = brd_lookup_page(brd, sector); This looks good. Unnecessary suggestion, but since you're already changing these, might as well replace "sector" with "idx" here and skip the duplicated shift in brd_lookup_page(). Reviewed-by: Keith Busch <kbusch@kernel.org>
On 3/15/24 19:12, Matthew Wilcox (Oracle) wrote: > This debugging check will become more costly in the future when we shrink > struct page. It has not proven to be useful, so simply remove it. > > This lets us use __xa_insert instead of __xa_cmpxchg() as we no longer > need to know about the page that is currently stored in the XArray. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > drivers/block/brd.c | 40 +++++++++++----------------------------- > 1 file changed, 11 insertions(+), 29 deletions(-) > > diff --git a/drivers/block/brd.c b/drivers/block/brd.c > index e322cef6596b..b900fe9e0030 100644 > --- a/drivers/block/brd.c > +++ b/drivers/block/brd.c > @@ -29,10 +29,7 @@ > > /* > * Each block ramdisk device has a xarray brd_pages of pages that stores > - * the pages containing the block device's contents. A brd page's ->index is > - * its offset in PAGE_SIZE units. This is similar to, but in no way connected > - * with, the kernel's pagecache or buffer cache (which sit above our block > - * device). > + * the pages containing the block device's contents. > */ > struct brd_device { > int brd_number; > @@ -51,15 +48,7 @@ struct brd_device { > */ > static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > { > - pgoff_t idx; > - struct page *page; > - > - idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */ > - page = xa_load(&brd->brd_pages, idx); > - > - BUG_ON(page && page->index != idx); > - > - return page; > + return xa_load(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT); > } > > /* > @@ -67,8 +56,8 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > */ > static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) > { > - pgoff_t idx; > - struct page *page, *cur; > + pgoff_t idx = sector >> PAGE_SECTORS_SHIFT; > + struct page *page; > int ret = 0; > > page = brd_lookup_page(brd, sector); > @@ -80,23 +69,16 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) > return -ENOMEM; > > xa_lock(&brd->brd_pages); > - > - idx = sector >> PAGE_SECTORS_SHIFT; > - page->index = idx; > - > - cur = __xa_cmpxchg(&brd->brd_pages, idx, NULL, page, gfp); > - > - if (unlikely(cur)) { > - __free_page(page); > - ret = xa_err(cur); > - if (!ret && (cur->index != idx)) > - ret = -EIO; > - } else { > + ret = __xa_insert(&brd->brd_pages, idx, page, gfp); > + if (!ret) > brd->brd_nr_pages++; > - } > - > xa_unlock(&brd->brd_pages); > > + if (ret < 0) { > + __free_page(page); > + if (ret == -EBUSY) > + ret = 0; > + } > return ret; > } > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Fri, Mar 15, 2024 at 12:23:23PM -0600, Keith Busch wrote: > On Fri, Mar 15, 2024 at 06:12:09PM +0000, Matthew Wilcox (Oracle) wrote: > > static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > > { > > - pgoff_t idx; > > - struct page *page; > > - > > - idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */ > > - page = xa_load(&brd->brd_pages, idx); > > - > > - BUG_ON(page && page->index != idx); > > - > > - return page; > > + return xa_load(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT); > > } > > > > /* > > @@ -67,8 +56,8 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) > > */ > > static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) > > { > > - pgoff_t idx; > > - struct page *page, *cur; > > + pgoff_t idx = sector >> PAGE_SECTORS_SHIFT; > > + struct page *page; > > int ret = 0; > > > > page = brd_lookup_page(brd, sector); > > This looks good. Unnecessary suggestion, but since you're already > changing these, might as well replace "sector" with "idx" here and skip > the duplicated shift in brd_lookup_page(). Sorry forget that, all the other existing callers just want the sector_t.
On Fri, Mar 15, 2024 at 12:27:31PM -0600, Keith Busch wrote: > On Fri, Mar 15, 2024 at 12:23:23PM -0600, Keith Busch wrote: > > This looks good. Unnecessary suggestion, but since you're already > > changing these, might as well replace "sector" with "idx" here and skip > > the duplicated shift in brd_lookup_page(). > > Sorry forget that, all the other existing callers just want the > sector_t. Yeah, but I could just inline the xa_load(). Doesn't really seem like a big deal to me, so I left it for now. Got to leave something for other people to do ;-)
On Fri, 15 Mar 2024 18:12:09 +0000, Matthew Wilcox (Oracle) wrote: > This debugging check will become more costly in the future when we shrink > struct page. It has not proven to be useful, so simply remove it. > > This lets us use __xa_insert instead of __xa_cmpxchg() as we no longer > need to know about the page that is currently stored in the XArray. > > > [...] Applied, thanks! [1/1] brd: Remove use of page->index commit: 3d2496fd32c27ddaf530356a9284ac0235c9d9ce Best regards,
diff --git a/drivers/block/brd.c b/drivers/block/brd.c index e322cef6596b..b900fe9e0030 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -29,10 +29,7 @@ /* * Each block ramdisk device has a xarray brd_pages of pages that stores - * the pages containing the block device's contents. A brd page's ->index is - * its offset in PAGE_SIZE units. This is similar to, but in no way connected - * with, the kernel's pagecache or buffer cache (which sit above our block - * device). + * the pages containing the block device's contents. */ struct brd_device { int brd_number; @@ -51,15 +48,7 @@ struct brd_device { */ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) { - pgoff_t idx; - struct page *page; - - idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */ - page = xa_load(&brd->brd_pages, idx); - - BUG_ON(page && page->index != idx); - - return page; + return xa_load(&brd->brd_pages, sector >> PAGE_SECTORS_SHIFT); } /* @@ -67,8 +56,8 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) */ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) { - pgoff_t idx; - struct page *page, *cur; + pgoff_t idx = sector >> PAGE_SECTORS_SHIFT; + struct page *page; int ret = 0; page = brd_lookup_page(brd, sector); @@ -80,23 +69,16 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) return -ENOMEM; xa_lock(&brd->brd_pages); - - idx = sector >> PAGE_SECTORS_SHIFT; - page->index = idx; - - cur = __xa_cmpxchg(&brd->brd_pages, idx, NULL, page, gfp); - - if (unlikely(cur)) { - __free_page(page); - ret = xa_err(cur); - if (!ret && (cur->index != idx)) - ret = -EIO; - } else { + ret = __xa_insert(&brd->brd_pages, idx, page, gfp); + if (!ret) brd->brd_nr_pages++; - } - xa_unlock(&brd->brd_pages); + if (ret < 0) { + __free_page(page); + if (ret == -EBUSY) + ret = 0; + } return ret; }
This debugging check will become more costly in the future when we shrink struct page. It has not proven to be useful, so simply remove it. This lets us use __xa_insert instead of __xa_cmpxchg() as we no longer need to know about the page that is currently stored in the XArray. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- drivers/block/brd.c | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-)