| Message ID | 20230216151918.319585-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support REQ_NWOAIT in brd | expand |
On 2/16/23 8:19?AM, Jens Axboe wrote: > @@ -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.
This should probably go into the previous patch, as that's the one creating the warning.
On 2/16/23 9:07 AM, Christoph Hellwig wrote: > 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.
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/202302170236.SynZo1Bx-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302170236.SynZo1Bx-lkp@intel.com/
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
diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 1ddada0cdaca..6019ef23344f 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -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++; }
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: stable@vger.kernel.org # 5.10+ Signed-off-by: Jens Axboe <axboe@kernel.dk> --- drivers/block/brd.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)