Skip to contentSkip to conversation

Support REQ_NWOAIT in brd

PATCH In review Jens Axboe 4 participants 16 messages Feb 16, 2023 – Feb 17, 2023

Subsystems (3)
Stable branch Block layer Ramdisk RAM block device driver
lore
Share & tools
lore

Jens Axboe

author maintainer
#0 lore raw

cover Support REQ_NWOAIT in brd

Hi,

Support non-blocking request issue for brd. Patches 1..3 are just prep work and cleanups in propagating the gfp mask correctly and dealing with errors that can happen, patch 4 enables nowait on the queue.

Signature · 1 line
Jens Axboe
author maintainer
#1 lore raw

PATCH 1/4 brd: return 0/-error from brd_insert_page()

It currently returns a page, but callers just check for NULL/page to gauge success. Clean this up and return the appropriate error directly instead.

Cc [email protected] # 5.10+
Signed-off-by Jens Axboe <[email protected]>
---
 drivers/block/brd.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/block/brd.c +12 -14
index 20acc4a1fd6d..15a148d5aad9 100644
@@ -78,11 +78,9 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
 }
 
 /*
- * Look up and return a brd's page for a given sector.
- * If one does not exist, allocate an empty page, and insert that. Then
- * return it.
+ * Insert a new page for a given sector, if one does not already exist.
  */
-static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
+static int brd_insert_page(struct brd_device *brd, sector_t sector)
 {
 	pgoff_t idx;
 	struct page *page;

@@ -90,7 +88,7 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
 
 	page = brd_lookup_page(brd, sector);
 	if (page)
-		return page;
+		return 0;
 
 	/*
 	 * Must use NOIO because we don't want to recurse back into the

@@ -99,11 +97,11 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
 	gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM;
 	page = alloc_page(gfp_flags);
 	if (!page)
-		return NULL;
+		return -ENOMEM;
 
 	if (radix_tree_preload(GFP_NOIO)) {
 		__free_page(page);
-		return NULL;
+		return -ENOMEM;
 	}
 
 	spin_lock(&brd->brd_lock);

@@ -120,8 +118,7 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
 	spin_unlock(&brd->brd_lock);
 
 	radix_tree_preload_end();
-
-	return page;
+	return 0;
 }
 
 /*

@@ -174,16 +171,17 @@ static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n)
 {
 	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
 	size_t copy;
+	int ret;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
-	if (!brd_insert_page(brd, sector))
-		return -ENOSPC;
+	ret = brd_insert_page(brd, sector);
+	if (ret)
+		return ret;
 	if (copy < n) {
 		sector += copy >> SECTOR_SHIFT;
-		if (!brd_insert_page(brd, sector))
-			return -ENOSPC;
+		ret = brd_insert_page(brd, sector);
 	}
-	return 0;
+	return ret;
 }
 
 /*
Signature · 1 line
2.39.1
#4 lore raw

Re: [PATCH 1/4] brd: return 0/-error from brd_insert_page()

Quoted 10 lines · Jens Axboe
Quoted 7 lines · Christoph Hellwig

Looks good:

Reviewed-by Christoph Hellwig <[email protected]>
Quoted 1 line
Cc [email protected] # 5.10+

But why is this a stable candidate?

Only because the other patches depend on it.

But none of those is stable material either as I can tell. It's a fairly simple and nice to have enhancement, but not really a grave bug or regression fix.

author maintainer
#5 lore raw

Re: [PATCH 1/4] brd: return 0/-error from brd_insert_page()

Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Christoph Hellwig

Looks good:

Reviewed-by Christoph Hellwig <[email protected]>
Quoted 1 line
Cc [email protected] # 5.10+
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

But why is this a stable candidate?

Only because the other patches depend on it.

But none of those is stable material either as I can tell. It's a fairly simple and nice to have enhancement, but not really a grave bug or regression fix.

It causes a big perf regression when swapping between IO backends, and even caused confusion for that initial reporter. So while it's not a very important crash fix, I do think shoving into stable is prudent.

Signature · 1 line
Jens Axboe
author maintainer
#7 lore raw

PATCH 2/4 brd: check for REQ_NOWAIT and set correct page allocation mask

In reply to Jens Axboe

If REQ_NOWAIT is set, then do a non-blocking allocation if the operation is a write and we need to insert a new page. Currently REQ_NOWAIT cannot be set as the queue isn't marked as supporting nowait, this change is in preparation for allowing that.

Cc [email protected] # 5.10+
Signed-off-by Jens Axboe <[email protected]>
---
 drivers/block/brd.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)
drivers/block/brd.c +22 -17
index 15a148d5aad9..1ddada0cdaca 100644
@@ -80,26 +80,20 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
 /*
  * Insert a new page for a given sector, if one does not already exist.
  */
-static int brd_insert_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;
-	gfp_t gfp_flags;
 
 	page = brd_lookup_page(brd, sector);
 	if (page)
 		return 0;
 
-	/*
-	 * Must use NOIO because we don't want to recurse back into the
-	 * block or filesystem layers from page reclaim.
-	 */
-	gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM;
-	page = alloc_page(gfp_flags);
+	page = alloc_page(gfp | __GFP_ZERO | __GFP_HIGHMEM);
 	if (!page)
 		return -ENOMEM;
 
-	if (radix_tree_preload(GFP_NOIO)) {
+	if (radix_tree_preload(gfp)) {
 		__free_page(page);
 		return -ENOMEM;
 	}

@@ -167,19 +161,20 @@ static void brd_free_pages(struct brd_device *brd)
 /*
  * copy_to_brd_setup must be called before copy_to_brd. It may sleep.
  */
-static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n)
+static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n,
+			     gfp_t gfp)
 {
 	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
 	size_t copy;
 	int ret;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
-	ret = brd_insert_page(brd, sector);
+	ret = brd_insert_page(brd, sector, gfp);
 	if (ret)
 		return ret;
 	if (copy < n) {
 		sector += copy >> SECTOR_SHIFT;
-		ret = brd_insert_page(brd, sector);
+		ret = brd_insert_page(brd, sector, gfp);
 	}
 	return ret;
 }

@@ -254,20 +249,26 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
  * Process a single bvec of a bio.
  */
 static int brd_do_bvec(struct brd_device *brd, struct page *page,
-			unsigned int len, unsigned int off, enum req_op op,
+			unsigned int len, unsigned int off, blk_opf_t opf,
 			sector_t sector)
 {
 	void *mem;
 	int err = 0;
 
-	if (op_is_write(op)) {
-		err = copy_to_brd_setup(brd, sector, len);
+	if (op_is_write(opf)) {
+		/*
+		 * Must use NOIO because we don't want to recurse back into the
+		 * block or filesystem layers from page reclaim.
+		 */
+		gfp_t gfp = opf & REQ_NOWAIT ? GFP_NOWAIT : GFP_NOIO;
+
+		err = copy_to_brd_setup(brd, sector, len, gfp);
 		if (err)
 			goto out;
 	}
 
 	mem = kmap_atomic(page);
-	if (!op_is_write(op)) {
+	if (!op_is_write(opf)) {
 		copy_from_brd(mem + off, brd, sector, len);
 		flush_dcache_page(page);
 	} else {

@@ -296,8 +297,12 @@ static void brd_submit_bio(struct bio *bio)
 				(len & (SECTOR_SIZE - 1)));
 
 		err = brd_do_bvec(brd, bvec.bv_page, len, bvec.bv_offset,
-				  bio_op(bio), sector);
+				  bio->bi_opf, sector);
 		if (err) {
+			if (err == -ENOMEM && bio->bi_opf & REQ_NOWAIT) {
+				bio_wouldblock_error(bio);
+				return;
+			}
 			bio_io_error(bio);
 			return;
 		}
Signature · 1 line
2.39.1
author maintainer
#10 lore raw

PATCH 3/4 brd: only preload radix tree if we're using a blocking gfp mask

In reply to Jens Axboe

radix_tree_preload() warns on attempting to call it with an allocation mask that doesn't allow blocking. While that warning could arguably be removed, we need to handle radix insertion failures anyway as they are more likely if we cannot block to get memory.

Remove legacy BUG_ON()'s and turn them into proper errors instead, one for the allocation failure and one for finding a page that doesn't match the correct index.

Cc [email protected] # 5.10+
Signed-off-by Jens Axboe <[email protected]>
---
 drivers/block/brd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/block/brd.c +6 -3
index 1ddada0cdaca..6019ef23344f 100644
@@ -84,6 +84,7 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp)
 {
 	pgoff_t idx;
 	struct page *page;
+	int ret = 0;
 
 	page = brd_lookup_page(brd, sector);
 	if (page)

@@ -93,7 +94,7 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp)
 	if (!page)
 		return -ENOMEM;
 
-	if (radix_tree_preload(gfp)) {
+	if (gfpflags_allow_blocking(gfp) && radix_tree_preload(gfp)) {
 		__free_page(page);
 		return -ENOMEM;
 	}

@@ -104,8 +105,10 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp)
 	if (radix_tree_insert(&brd->brd_pages, idx, page)) {
 		__free_page(page);
 		page = radix_tree_lookup(&brd->brd_pages, idx);
-		BUG_ON(!page);
-		BUG_ON(page->index != idx);
+		if (!page)
+			ret = -ENOMEM;
+		else if (page->index != idx)
+			ret = -EIO;
 	} else {
 		brd->brd_nr_pages++;
 	}
Signature · 1 line
2.39.1
author maintainer
#11 lore raw

Re: [PATCH 3/4] brd: only preload radix tree if we're using a blocking gfp mask

drivers/block/brd.c static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) · line 114

Quoted 13 lines · Jens Axboe
drivers/block/brd.c +4 -2
@@ -104,8 +105,10 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp)
 	if (radix_tree_insert(&brd->brd_pages, idx, page)) {
 		__free_page(page);
 		page = radix_tree_lookup(&brd->brd_pages, idx);
-		BUG_ON(!page);
-		BUG_ON(page->index != idx);
+		if (!page)
+			ret = -ENOMEM;
+		else if (page->index != idx)
+			ret = -EIO;
 	} else {
 		brd->brd_nr_pages++;
 	}

After sending this out, noticed that I forgot to change the return 0 to return ret instead. This has been done locally, fwiw.

Signature · 1 line
Jens Axboe
#12 lore raw

Re: [PATCH 3/4] brd: only preload radix tree if we're using a blocking gfp mask

In reply to Jens Axboe

This should probably go into the previous patch, as that's the one creating the warning.

author maintainer
#13 lore raw

Re: [PATCH 3/4] brd: only preload radix tree if we're using a blocking gfp mask

Quoted 2 lines · Christoph Hellwig

This should probably go into the previous patch, as that's the one creating the warning.

Agree, but it also can't happen before patch 4. But I can fold the two, only did the split because it's nice to have the error handling separate.

Signature · 1 line
Jens Axboe
build bot
#14 lore raw

Re: [PATCH 3/4] brd: only preload radix tree if we're using a blocking gfp mask

In reply to Jens Axboe
I love your patch! Perhaps something to improve:

Hi Jens,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v6.2-rc8]
[also build test WARNING on linus/master next-20230216] [cannot apply to axboe-block/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jens-Axboe/brd-return-0-error-from-brd_insert_page/20230216-234430
patch link:    https://lore.kernel.org/r/20230216151918.319585-4-axboe%40kernel.dk
patch subject: [PATCH 3/4] brd: only preload radix tree if we're using a blocking gfp mask
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230217/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/237074d417b50a21f2bed5585ceebe8398535b1a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jens-Axboe/brd-return-0-error-from-brd_insert_page/20230216-234430 git checkout 237074d417b50a21f2bed5585ceebe8398535b1a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/block/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

drivers/block/brd.c: In function 'brd_insert_page':

>> drivers/block/brd.c:87:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
      87 |         int ret = 0;
         |             ^~~

vim +/ret +87 drivers/block/brd.c

    79	
    80	/*
    81	 * Insert a new page for a given sector, if one does not already exist.
    82	 */
    83	static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp)
    84	{
    85		pgoff_t idx;
    86		struct page *page;
  > 87		int ret = 0;
    88	
    89		page = brd_lookup_page(brd, sector);
    90		if (page)
    91			return 0;
    92	
    93		page = alloc_page(gfp | __GFP_ZERO | __GFP_HIGHMEM);
    94		if (!page)
    95			return -ENOMEM;
    96	
    97		if (gfpflags_allow_blocking(gfp) && radix_tree_preload(gfp)) {
    98			__free_page(page);
    99			return -ENOMEM;

100 }
101
102 spin_lock(&brd->brd_lock);
103 idx = sector >> PAGE_SECTORS_SHIFT;
104 page->index = idx;
105 if (radix_tree_insert(&brd->brd_pages, idx, page)) {
106 __free_page(page);
107 page = radix_tree_lookup(&brd->brd_pages, idx);
108 if (!page)
109 ret = -ENOMEM;
110 else if (page->index != idx)
111 ret = -EIO;
112 } else {
113 brd->brd_nr_pages++;
114 }
115 spin_unlock(&brd->brd_lock);
116
117 radix_tree_preload_end();
118 return 0;
119 }
120

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
author maintainer
#15 lore raw

PATCH 4/4 brd: mark as nowait compatible

In reply to Jens Axboe

By default, non-mq drivers do not support nowait. This causes io_uring to use a slower path as the driver cannot be trust not to block. brd can safely set the nowait flag, as worst case all it does is a NOIO allocation.

For io_uring, this makes a substantial difference. Before:

submitter=0, tid=453, file=/dev/ram0, node=-1 polled=0, fixedbufs=1/0, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=440.03K, BW=1718MiB/s, IOS/call=32/31 IOPS=428.96K, BW=1675MiB/s, IOS/call=32/32 IOPS=442.59K, BW=1728MiB/s, IOS/call=32/31 IOPS=419.65K, BW=1639MiB/s, IOS/call=32/32 IOPS=426.82K, BW=1667MiB/s, IOS/call=32/31

and after:

submitter=0, tid=354, file=/dev/ram0, node=-1 polled=0, fixedbufs=1/0, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=3.37M, BW=13.15GiB/s, IOS/call=32/31 IOPS=3.45M, BW=13.46GiB/s, IOS/call=32/31 IOPS=3.43M, BW=13.42GiB/s, IOS/call=32/32 IOPS=3.43M, BW=13.39GiB/s, IOS/call=32/31 IOPS=3.43M, BW=13.38GiB/s, IOS/call=32/31

or about an 8x in difference. Now that brd is prepared to deal with REQ_NOWAIT reads/writes, mark it as supporting that.

Cc [email protected] # 5.10+
Link https://lore.kernel.org/linux-block/[email protected]/ open here
Signed-off-by Jens Axboe <[email protected]>
---
 drivers/block/brd.c | 1 +
 1 file changed, 1 insertion(+)
drivers/block/brd.c +1 -0
index 6019ef23344f..522530a6ebca 100644
@@ -418,6 +418,7 @@ static int brd_alloc(int i)
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue);
+	blk_queue_flag_set(QUEUE_FLAG_NOWAIT, disk->queue);
 	err = add_disk(disk);
 	if (err)
 		goto out_cleanup_disk;
Signature · 1 line
2.39.1