[PATCH] brd: Remove use of page->index

[PATCH] brd: Remove use of page->index

[Matthew Wilcox (Oracle)]

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;
 }
 
-- 
2.43.0

Re: [PATCH] brd: Remove use of page->index

[Keith Busch]

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>

Re: [PATCH] brd: Remove use of page->index

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] brd: Remove use of page->index

[Keith Busch]

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.

Re: [PATCH] brd: Remove use of page->index

[Matthew Wilcox]

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 ;-)

Re: [PATCH] brd: Remove use of page->index

[Jens Axboe]

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,
-- 
Jens Axboe