[PATCH] bcache: set io_disable to true when stop bcache device

[PATCH] bcache: set io_disable to true when stop bcache device

[mingzhe]

Stop is an operation that cannot be aborted. If there are still
IO requests being processed, we can never stop the device.
So, all new IO requests should fail when we set io_disable to true.
However, sysfs has been unlinked at this time, user cannot modify
io_disable via sysfs.

Signed-off-by: mingzhe <mingzhe.zou@easystack.cn>
---
 drivers/md/bcache/request.c | 16 ++++++++++++++++
 drivers/md/bcache/super.c   |  9 +++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 67a2e29e0b40..9b85aad20022 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -758,6 +758,15 @@ static void cached_dev_bio_complete(struct closure *cl)
 	search_free(cl);
 }
 
+static void cached_dev_bio_fail(struct closure *cl)
+{
+	struct search *s = container_of(cl, struct search, cl);
+	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
+
+	s->iop.status = BLK_STS_IOERR;
+	cached_dev_bio_complete(cl);
+}
+
 /* Process reads */
 
 static void cached_dev_read_error_done(struct closure *cl)
@@ -971,6 +980,9 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
 	struct bkey start = KEY(dc->disk.id, bio->bi_iter.bi_sector, 0);
 	struct bkey end = KEY(dc->disk.id, bio_end_sector(bio), 0);
 
+	if (unlikely((dc->io_disable)))
+		goto fail_bio;
+
 	bch_keybuf_check_overlapping(&s->iop.c->moving_gc_keys, &start, &end);
 
 	down_read_non_owner(&dc->writeback_lock);
@@ -1046,6 +1058,10 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
 insert_data:
 	closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
 	continue_at(cl, cached_dev_write_complete, NULL);
+	return;
+
+fail_bio:
+	continue_at(cl, cached_dev_bio_fail, NULL);
 }
 
 static void cached_dev_nodata(struct closure *cl)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index ba3909bb6bea..a2a82942f85b 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1389,6 +1389,15 @@ static void cached_dev_flush(struct closure *cl)
 	bch_cache_accounting_destroy(&dc->accounting);
 	kobject_del(&d->kobj);
 
+	/*
+	 * Stop is an operation that cannot be aborted. If there are still
+	 * IO requests being processed, we can never stop the device.
+	 * So, all new IO requests should fail when we set io_disable to true.
+	 * However, sysfs has been unlinked at this time, user cannot modify
+	 * io_disable via sysfs.
+	 */
+	dc->io_disable = true;
+
 	continue_at(cl, cached_dev_free, system_wq);
 }
 
-- 
2.17.1.windows.2

Re: [PATCH] bcache: set io_disable to true when stop bcache device

[Coly Li]

> 2023年3月8日 17:20，mingzhe <mingzhe.zou@easystack.cn> 写道：
> 
> Stop is an operation that cannot be aborted. If there are still
> IO requests being processed, we can never stop the device.
> So, all new IO requests should fail when we set io_disable to true.
> However, sysfs has been unlinked at this time, user cannot modify
> io_disable via sysfs.
> 
> Signed-off-by: mingzhe <mingzhe.zou@easystack.cn>

NACK. I tried this years ago, it might introduce potential risk for meta data inconsistency.


Coly Li


> ---
> drivers/md/bcache/request.c | 16 ++++++++++++++++
> drivers/md/bcache/super.c   |  9 +++++++++
> 2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 67a2e29e0b40..9b85aad20022 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -758,6 +758,15 @@ static void cached_dev_bio_complete(struct closure *cl)
> search_free(cl);
> }
> 
> +static void cached_dev_bio_fail(struct closure *cl)
> +{
> + struct search *s = container_of(cl, struct search, cl);
> + struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
> +
> + s->iop.status = BLK_STS_IOERR;
> + cached_dev_bio_complete(cl);
> +}
> +
> /* Process reads */
> 
> static void cached_dev_read_error_done(struct closure *cl)
> @@ -971,6 +980,9 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
> struct bkey start = KEY(dc->disk.id, bio->bi_iter.bi_sector, 0);
> struct bkey end = KEY(dc->disk.id, bio_end_sector(bio), 0);
> 
> + if (unlikely((dc->io_disable)))
> + goto fail_bio;
> +
> bch_keybuf_check_overlapping(&s->iop.c->moving_gc_keys, &start, &end);
> 
> down_read_non_owner(&dc->writeback_lock);
> @@ -1046,6 +1058,10 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
> insert_data:
> closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
> continue_at(cl, cached_dev_write_complete, NULL);
> + return;
> +
> +fail_bio:
> + continue_at(cl, cached_dev_bio_fail, NULL);
> }
> 
> static void cached_dev_nodata(struct closure *cl)
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index ba3909bb6bea..a2a82942f85b 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1389,6 +1389,15 @@ static void cached_dev_flush(struct closure *cl)
> bch_cache_accounting_destroy(&dc->accounting);
> kobject_del(&d->kobj);
> 
> + /*
> + * Stop is an operation that cannot be aborted. If there are still
> + * IO requests being processed, we can never stop the device.
> + * So, all new IO requests should fail when we set io_disable to true.
> + * However, sysfs has been unlinked at this time, user cannot modify
> + * io_disable via sysfs.
> + */
> + dc->io_disable = true;
> +
> continue_at(cl, cached_dev_free, system_wq);
> }
> 
> -- 
> 2.17.1.windows.2
> 

Re: [PATCH] bcache: set io_disable to true when stop bcache device

[kernel test robot]

Hi mingzhe,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc1 next-20230308]
[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/mingzhe/bcache-set-io_disable-to-true-when-stop-bcache-device/20230308-172245
patch link:    https://lore.kernel.org/r/20230308092036.11024-1-mingzhe.zou%40easystack.cn
patch subject: [PATCH] bcache: set io_disable to true when stop bcache device
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303082025.PP1BZsDY-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/6a59a84151f42d58e6010f18c92b98cbaf58bdc1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review mingzhe/bcache-set-io_disable-to-true-when-stop-bcache-device/20230308-172245
        git checkout 6a59a84151f42d58e6010f18c92b98cbaf58bdc1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/md/

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/202303082025.PP1BZsDY-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/md/bcache/request.c: In function 'cached_dev_bio_fail':
>> drivers/md/bcache/request.c:764:28: warning: unused variable 'dc' [-Wunused-variable]
     764 |         struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
         |                            ^~


vim +/dc +764 drivers/md/bcache/request.c

   760	
   761	static void cached_dev_bio_fail(struct closure *cl)
   762	{
   763		struct search *s = container_of(cl, struct search, cl);
 > 764		struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
   765	
   766		s->iop.status = BLK_STS_IOERR;
   767		cached_dev_bio_complete(cl);
   768	}
   769	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests