[dm-devel] [PATCH] dm table: Constify static struct blk_ksm_ll_ops
The only usage of dm_ksm_ll_ops is to make a copy of it to the ksm_ll_ops field in the blk_keyslot_manager struct. Make it const to allow the compiler to put it in read-only memory. Signed-off-by: Rikard Falkeborn --- drivers/md/dm-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index ee47a332b462..7e88e5e06922 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -1244,7 +1244,7 @@ static int dm_keyslot_evict(struct blk_keyslot_manager *ksm, return args.err; } -static struct blk_ksm_ll_ops dm_ksm_ll_ops = { +static const struct blk_ksm_ll_ops dm_ksm_ll_ops = { .keyslot_evict = dm_keyslot_evict, }; -- 2.31.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations
On 2021/05/26 23:16, Mikulas Patocka wrote: > > > On Tue, 25 May 2021, Damien Le Moal wrote: > >> On 2021/05/26 4:50, Mikulas Patocka wrote: >>> The functions set_bit and clear_bit are atomic. We don't need atomicity >>> when making flags for dm-kcopyd. So, change them to direct manipulation of >>> the flags. >>> >>> Signed-off-by: Mikulas Patocka >>> >>> Index: linux-2.6/drivers/md/dm-kcopyd.c >>> === >>> --- linux-2.6.orig/drivers/md/dm-kcopyd.c >>> +++ linux-2.6/drivers/md/dm-kcopyd.c >>> @@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli >>> if (!test_bit(DM_KCOPYD_WRITE_SEQ, >flags)) { >>> for (i = 0; i < job->num_dests; i++) { >>> if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { >>> - set_bit(DM_KCOPYD_WRITE_SEQ, >flags); >>> + job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ; >> >> How about using the BIT() macro ? >> >> job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); >> >> But I know some do not like that macro :) > > Yes - it is better to use it. > I also changed flags from unsigned long to unsigned, to make the structure > smaller. > > > From: Mikulas Patocka > > dm-kcopyd: avoid useless atomic operations > > The functions set_bit and clear_bit are atomic. We don't need atomicity > when making flags for dm-kcopyd. So, change them to direct manipulation of > the flags. > > Signed-off-by: Mikulas Patocka > > Index: linux-2.6/drivers/md/dm-kcopyd.c > === > --- linux-2.6.orig/drivers/md/dm-kcopyd.c > +++ linux-2.6/drivers/md/dm-kcopyd.c > @@ -341,7 +341,7 @@ static void client_free_pages(struct dm_ > struct kcopyd_job { > struct dm_kcopyd_client *kc; > struct list_head list; > - unsigned long flags; > + unsigned flags; This triggers a checkpatch warning. "unsigned int" would be better. > > /* >* Error state of the job. > @@ -418,7 +418,7 @@ static struct kcopyd_job *pop_io_job(str >* constraint and sequential writes that are at the right position. >*/ > list_for_each_entry(job, jobs, list) { > - if (job->rw == READ || !test_bit(DM_KCOPYD_WRITE_SEQ, > >flags)) { > + if (job->rw == READ || !(job->flags & > BIT(DM_KCOPYD_WRITE_SEQ))) { > list_del(>list); > return job; > } > @@ -529,7 +529,7 @@ static void complete_io(unsigned long er > else > job->read_err = 1; > > - if (!test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) { > + if (!(job->flags & BIT(DM_KCOPYD_IGNORE_ERROR))) { > push(>complete_jobs, job); > wake(kc); > return; > @@ -572,7 +572,7 @@ static int run_io_job(struct kcopyd_job >* If we need to write sequentially and some reads or writes failed, >* no point in continuing. >*/ > - if (test_bit(DM_KCOPYD_WRITE_SEQ, >flags) && > + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && > job->master_job->write_err) { > job->write_err = job->master_job->write_err; > return -EIO; > @@ -716,7 +716,7 @@ static void segment_complete(int read_er >* Only dispatch more work if there hasn't been an error. >*/ > if ((!job->read_err && !job->write_err) || > - test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) { > + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) { > /* get the next chunk of work */ > progress = job->progress; > count = job->source.count - progress; > @@ -809,10 +809,10 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli >* we need to write sequentially. If one of the destination is a >* host-aware device, then leave it to the caller to choose what to do. >*/ > - if (!test_bit(DM_KCOPYD_WRITE_SEQ, >flags)) { > + if (!(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { > for (i = 0; i < job->num_dests; i++) { > if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { > - set_bit(DM_KCOPYD_WRITE_SEQ, >flags); > + job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); > break; > } > } > @@ -821,9 +821,9 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > /* >* If we need to write sequentially, errors cannot be ignored. >*/ > - if (test_bit(DM_KCOPYD_WRITE_SEQ, >flags) && > - test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) > - clear_bit(DM_KCOPYD_IGNORE_ERROR, >flags); > + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && > + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) > + job->flags &= ~BIT(DM_KCOPYD_IGNORE_ERROR); > > if (from) { > job->source = *from; > Index:
Re: [dm-devel] [PATCH] dm-kcopyd avoid spin_lock_irqsave from process context
On 2021/05/26 23:18, Mikulas Patocka wrote: > The functions "pop", "push_head", "do_work" can only be called from > process context. Therefore, we can replace > spin_lock_irqsave/spin_unlock_irqrestore with > spin_lock_irq/spin_unlock_irq. > > Signed-off-by: Mikulas Patocka Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [dm:for-next 10/11] drivers/md/dm-kcopyd.c:538:42: error: passing argument 2 of 'test_bit' from incompatible pointer type
tree: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next head: 02e71d9bee03399bac6869d9afba8665650ad20a commit: 1e754b11d1c0c22516822481ba26592d5fb1ef9a [10/11] dm: improve kcopyd latency config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.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://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?id=1e754b11d1c0c22516822481ba26592d5fb1ef9a git remote add dm https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git git fetch --no-tags dm for-next git checkout 1e754b11d1c0c22516822481ba26592d5fb1ef9a # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): drivers/md/dm-kcopyd.c: In function 'complete_io': >> drivers/md/dm-kcopyd.c:538:42: error: passing argument 2 of 'test_bit' from >> incompatible pointer type [-Werror=incompatible-pointer-types] 538 | if (test_bit(DM_KCOPYD_EARLY_CALLBACK, >flags)) { | ^~~ | | | unsigned int * In file included from arch/sh/include/asm/bitops.h:24, from include/linux/bitops.h:32, from include/linux/kernel.h:12, from include/asm-generic/bug.h:20, from arch/sh/include/asm/bug.h:112, from include/linux/bug.h:5, from include/linux/thread_info.h:12, from include/asm-generic/current.h:5, from ./arch/sh/include/generated/asm/current.h:1, from include/linux/sched.h:12, from include/linux/blkdev.h:5, from drivers/md/dm-kcopyd.c:14: include/asm-generic/bitops/non-atomic.h:104:66: note: expected 'const volatile long unsigned int *' but argument is of type 'unsigned int *' 104 | static inline int test_bit(int nr, const volatile unsigned long *addr) |~~^~~~ cc1: some warnings being treated as errors Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA Selected by - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC vim +/test_bit +538 drivers/md/dm-kcopyd.c 516 517 static void complete_io(unsigned long error, void *context) 518 { 519 struct kcopyd_job *job = (struct kcopyd_job *) context; 520 struct dm_kcopyd_client *kc = job->kc; 521 522 io_job_finish(kc->throttle); 523 524 if (error) { 525 if (op_is_write(job->rw)) 526 job->write_err |= error; 527 else 528 job->read_err = 1; 529 530 if (!(job->flags & BIT(DM_KCOPYD_IGNORE_ERROR))) { 531 push(>complete_jobs, job); 532 wake(kc); 533 return; 534 } 535 } 536 537 if (op_is_write(job->rw)) { > 538 if (test_bit(DM_KCOPYD_EARLY_CALLBACK, >flags)) { 539 job->fn(job->read_err, job->write_err, job->context); 540 job->fn = null_completion; 541 } 542 push(>complete_jobs, job); 543 } else { 544 job->rw = WRITE; 545 push(>io_jobs, job); 546 } 547 548 wake(kc); 549 } 550 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm-writecace: if we are suspended, interrupt flushing
Hi This is another writecache patch, intended for the next merge window. Mikulas From: Mikulas Patocka If we are suspended, we want to interrupt the flushing sequence, so that there is no excessive suspend delay. Signed-off-by: Mikulas Patocka Cc: sta...@vger.kernel.org Index: linux-2.6/drivers/md/dm-writecache.c === --- linux-2.6.orig/drivers/md/dm-writecache.c +++ linux-2.6/drivers/md/dm-writecache.c @@ -1844,8 +1844,9 @@ restart: n_walked++; if (unlikely(n_walked > WRITEBACK_LATENCY) && - likely(!wc->writeback_all) && likely(!dm_suspended(wc->ti))) { - queue_work(wc->writeback_wq, >writeback_work); + likely(!wc->writeback_all)) { + if (likely(!dm_suspended(wc->ti))) + queue_work(wc->writeback_wq, >writeback_work); break; } -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm-writecache: use early kcopyd callback
Hi You can add this to the kernel that will be used for testing writecache. Another suggestion is to try to use the dm table argument "writeback_jobs" - for example: dmsetup create wc --table "0 `blockdev --getsize /dev/vdb` writecache s /dev/vdb /dev/vdc 4096 2 writeback_jobs 65536" It limits the number of work that is submitted to kcopyd and it may improve latency. Mikulas Index: linux-2.6/drivers/md/dm-writecache.c === --- linux-2.6.orig/drivers/md/dm-writecache.c +++ linux-2.6/drivers/md/dm-writecache.c @@ -1538,14 +1538,15 @@ static void writecache_copy_endio(int re { struct copy_struct *c = ptr; struct dm_writecache *wc = c->wc; + unsigned long flags; c->error = likely(!(read_err | write_err)) ? 0 : -EIO; - raw_spin_lock_irq(>endio_list_lock); + raw_spin_lock_irqsave(>endio_list_lock, flags); if (unlikely(list_empty(>endio_list))) wake_up_process(wc->endio_thread); list_add_tail(>endio_entry, >endio_list); - raw_spin_unlock_irq(>endio_list_lock); + raw_spin_unlock_irqrestore(>endio_list_lock, flags); } static void __writecache_endio_pmem(struct dm_writecache *wc, struct list_head *list) @@ -1799,7 +1800,7 @@ static void __writecache_writeback_ssd(s from.count = to.count = wc->data_device_sectors - to.sector; } - dm_kcopyd_copy(wc->dm_kcopyd, , 1, , 0, writecache_copy_endio, c); + dm_kcopyd_copy(wc->dm_kcopyd, , 1, , BIT(DM_KCOPYD_EARLY_CALLBACK), writecache_copy_endio, c); __writeback_throttle(wc, wbl); } -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm-writecache: don't split bios on block boundary when overwriting cache content
If dm-writecache overwrote existing cached data, it would split the incoming bio to many block-sized bios. The I/O scheduler would reassemble these bios into one large request. This split and reassemble causes performance degradation. This patch avoids splitting the bio if the target area that is being overwritten is contiguous. Signed-off-by: Mikulas Patocka Index: linux-2.6/drivers/md/dm-writecache.c === --- linux-2.6.orig/drivers/md/dm-writecache.c +++ linux-2.6/drivers/md/dm-writecache.c @@ -1360,14 +1360,18 @@ read_next_block: } else { do { bool found_entry = false; + bool search_used = false; if (writecache_has_error(wc)) goto unlock_error; e = writecache_find_entry(wc, bio->bi_iter.bi_sector, 0); if (e) { - if (!writecache_entry_is_committed(wc, e)) + if (!writecache_entry_is_committed(wc, e)) { + search_used = true; goto bio_copy; + } if (!WC_MODE_PMEM(wc) && !e->write_in_progress) { wc->overwrote_committed = true; + search_used = true; goto bio_copy; } found_entry = true; @@ -1404,13 +1408,30 @@ bio_copy: sector_t current_cache_sec = start_cache_sec + (bio_size >> SECTOR_SHIFT); while (bio_size < bio->bi_iter.bi_size) { - struct wc_entry *f = writecache_pop_from_freelist(wc, current_cache_sec); - if (!f) - break; - write_original_sector_seq_count(wc, f, bio->bi_iter.bi_sector + - (bio_size >> SECTOR_SHIFT), wc->seq_count); - writecache_insert_entry(wc, f); - wc->uncommitted_blocks++; + if (!search_used) { + struct wc_entry *f = writecache_pop_from_freelist(wc, current_cache_sec); + if (!f) + break; + write_original_sector_seq_count(wc, f, bio->bi_iter.bi_sector + + (bio_size >> SECTOR_SHIFT), wc->seq_count); + writecache_insert_entry(wc, f); + wc->uncommitted_blocks++; + } else { + struct wc_entry *f; + struct rb_node *next = rb_next(>rb_node); + if (!next) + break; + f = container_of(next, struct wc_entry, rb_node); + if (f != e + 1) + break; + if (read_original_sector(wc, f) != read_original_sector(wc, e) + (wc->block_size >> SECTOR_SHIFT)) + break; + if (unlikely(f->write_in_progress)) + break; + if (writecache_entry_is_committed(wc, f)) + wc->overwrote_committed = true; + e = f; + } bio_size += wc->block_size; current_cache_sec += wc->block_size >> SECTOR_SHIFT; } -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm-kcopyd avoid spin_lock_irqsave from process context
The functions "pop", "push_head", "do_work" can only be called from process context. Therefore, we can replace spin_lock_irqsave/spin_unlock_irqrestore with spin_lock_irq/spin_unlock_irq. Signed-off-by: Mikulas Patocka Index: linux-2.6/drivers/md/dm-kcopyd.c === --- linux-2.6.orig/drivers/md/dm-kcopyd.c +++ linux-2.6/drivers/md/dm-kcopyd.c @@ -437,9 +437,8 @@ static struct kcopyd_job *pop(struct lis struct dm_kcopyd_client *kc) { struct kcopyd_job *job = NULL; - unsigned long flags; - spin_lock_irqsave(>job_lock, flags); + spin_lock_irq(>job_lock); if (!list_empty(jobs)) { if (jobs == >io_jobs) @@ -449,7 +448,7 @@ static struct kcopyd_job *pop(struct lis list_del(>list); } } - spin_unlock_irqrestore(>job_lock, flags); + spin_unlock_irq(>job_lock); return job; } @@ -467,12 +466,11 @@ static void push(struct list_head *jobs, static void push_head(struct list_head *jobs, struct kcopyd_job *job) { - unsigned long flags; struct dm_kcopyd_client *kc = job->kc; - spin_lock_irqsave(>job_lock, flags); + spin_lock_irq(>job_lock); list_add(>list, jobs); - spin_unlock_irqrestore(>job_lock, flags); + spin_unlock_irq(>job_lock); } /* @@ -655,7 +653,6 @@ static void do_work(struct work_struct * struct dm_kcopyd_client *kc = container_of(work, struct dm_kcopyd_client, kcopyd_work); struct blk_plug plug; - unsigned long flags; /* * The order that these are called is *very* important. @@ -664,9 +661,9 @@ static void do_work(struct work_struct * * list. io jobs call wake when they complete and it all * starts again. */ - spin_lock_irqsave(>job_lock, flags); + spin_lock_irq(>job_lock); list_splice_tail_init(>callback_jobs, >complete_jobs); - spin_unlock_irqrestore(>job_lock, flags); + spin_unlock_irq(>job_lock); blk_start_plug(); process_jobs(>complete_jobs, kc, run_complete_job); -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations
On Tue, 25 May 2021, Damien Le Moal wrote: > On 2021/05/26 4:50, Mikulas Patocka wrote: > > The functions set_bit and clear_bit are atomic. We don't need atomicity > > when making flags for dm-kcopyd. So, change them to direct manipulation of > > the flags. > > > > Signed-off-by: Mikulas Patocka > > > > Index: linux-2.6/drivers/md/dm-kcopyd.c > > === > > --- linux-2.6.orig/drivers/md/dm-kcopyd.c > > +++ linux-2.6/drivers/md/dm-kcopyd.c > > @@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > > if (!test_bit(DM_KCOPYD_WRITE_SEQ, >flags)) { > > for (i = 0; i < job->num_dests; i++) { > > if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { > > - set_bit(DM_KCOPYD_WRITE_SEQ, >flags); > > + job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ; > > How about using the BIT() macro ? > > job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); > > But I know some do not like that macro :) Yes - it is better to use it. I also changed flags from unsigned long to unsigned, to make the structure smaller. From: Mikulas Patocka dm-kcopyd: avoid useless atomic operations The functions set_bit and clear_bit are atomic. We don't need atomicity when making flags for dm-kcopyd. So, change them to direct manipulation of the flags. Signed-off-by: Mikulas Patocka Index: linux-2.6/drivers/md/dm-kcopyd.c === --- linux-2.6.orig/drivers/md/dm-kcopyd.c +++ linux-2.6/drivers/md/dm-kcopyd.c @@ -341,7 +341,7 @@ static void client_free_pages(struct dm_ struct kcopyd_job { struct dm_kcopyd_client *kc; struct list_head list; - unsigned long flags; + unsigned flags; /* * Error state of the job. @@ -418,7 +418,7 @@ static struct kcopyd_job *pop_io_job(str * constraint and sequential writes that are at the right position. */ list_for_each_entry(job, jobs, list) { - if (job->rw == READ || !test_bit(DM_KCOPYD_WRITE_SEQ, >flags)) { + if (job->rw == READ || !(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { list_del(>list); return job; } @@ -529,7 +529,7 @@ static void complete_io(unsigned long er else job->read_err = 1; - if (!test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) { + if (!(job->flags & BIT(DM_KCOPYD_IGNORE_ERROR))) { push(>complete_jobs, job); wake(kc); return; @@ -572,7 +572,7 @@ static int run_io_job(struct kcopyd_job * If we need to write sequentially and some reads or writes failed, * no point in continuing. */ - if (test_bit(DM_KCOPYD_WRITE_SEQ, >flags) && + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && job->master_job->write_err) { job->write_err = job->master_job->write_err; return -EIO; @@ -716,7 +716,7 @@ static void segment_complete(int read_er * Only dispatch more work if there hasn't been an error. */ if ((!job->read_err && !job->write_err) || - test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) { + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) { /* get the next chunk of work */ progress = job->progress; count = job->source.count - progress; @@ -809,10 +809,10 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli * we need to write sequentially. If one of the destination is a * host-aware device, then leave it to the caller to choose what to do. */ - if (!test_bit(DM_KCOPYD_WRITE_SEQ, >flags)) { + if (!(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { for (i = 0; i < job->num_dests; i++) { if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { - set_bit(DM_KCOPYD_WRITE_SEQ, >flags); + job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); break; } } @@ -821,9 +821,9 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli /* * If we need to write sequentially, errors cannot be ignored. */ - if (test_bit(DM_KCOPYD_WRITE_SEQ, >flags) && - test_bit(DM_KCOPYD_IGNORE_ERROR, >flags)) - clear_bit(DM_KCOPYD_IGNORE_ERROR, >flags); + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) + job->flags &= ~BIT(DM_KCOPYD_IGNORE_ERROR); if (from) { job->source = *from; Index: linux-2.6/drivers/md/dm-raid1.c === --- linux-2.6.orig/drivers/md/dm-raid1.c +++ linux-2.6/drivers/md/dm-raid1.c @@ -364,7
Re: [dm-devel] [PATCH v4 02/11] block: introduce bio zone helpers
On 5/24/21 9:25 PM, Damien Le Moal wrote: Introduce the helper functions bio_zone_no() and bio_zone_is_seq(). Both are the BIO counterparts of the request helpers blk_rq_zone_no() and blk_rq_zone_is_seq(), respectively returning the number of the target zone of a bio and true if the BIO target zone is sequential. Signed-off-by: Damien Le Moal Reviewed-by: Hannes Reinecke Reviewed-by: Chaitanya Kulkarni --- include/linux/blkdev.h | 12 1 file changed, 12 insertions(+) diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index f69c75bd6d27..2db0f376f5d9 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1008,6 +1008,18 @@ static inline unsigned int blk_rq_stats_sectors(const struct request *rq) /* Helper to convert BLK_ZONE_ZONE_XXX to its string format XXX */ const char *blk_zone_cond_str(enum blk_zone_cond zone_cond); +static inline unsigned int bio_zone_no(struct bio *bio) +{ + return blk_queue_zone_no(bdev_get_queue(bio->bi_bdev), +bio->bi_iter.bi_sector); +} + +static inline unsigned int bio_zone_is_seq(struct bio *bio) +{ + return blk_queue_zone_is_seq(bdev_get_queue(bio->bi_bdev), +bio->bi_iter.bi_sector); +} + static inline unsigned int blk_rq_zone_no(struct request *rq) { return blk_queue_zone_no(rq->q, blk_rq_pos(rq)); Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 04/11] dm: Fix dm_accept_partial_bio()
On 5/24/21 9:25 PM, Damien Le Moal wrote: Fix dm_accept_partial_bio() to actually check that zone management commands are not passed as explained in the function documentation comment. Also, since a zone append operation cannot be split, add REQ_OP_ZONE_APPEND as a forbidden command. White lines are added around the group of BUG_ON() calls to make the code more legible. Signed-off-by: Damien Le Moal Reviewed-by: Johannes Thumshirn Reviewed-by: Hannes Reinecke --- drivers/md/dm.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/md/dm.c b/drivers/md/dm.c index ca2aedd8ee7d..a9211575bfed 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1237,8 +1237,9 @@ static int dm_dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff, /* * A target may call dm_accept_partial_bio only from the map routine. It is - * allowed for all bio types except REQ_PREFLUSH, REQ_OP_ZONE_RESET, - * REQ_OP_ZONE_OPEN, REQ_OP_ZONE_CLOSE and REQ_OP_ZONE_FINISH. + * allowed for all bio types except REQ_PREFLUSH, zone management operations + * (REQ_OP_ZONE_RESET, REQ_OP_ZONE_OPEN, REQ_OP_ZONE_CLOSE and + * REQ_OP_ZONE_FINISH) and zone append writes. * * dm_accept_partial_bio informs the dm that the target only wants to process * additional n_sectors sectors of the bio and the rest of the data should be @@ -1268,9 +1269,13 @@ void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors) { struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone); unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT; + BUG_ON(bio->bi_opf & REQ_PREFLUSH); + BUG_ON(op_is_zone_mgmt(bio_op(bio))); + BUG_ON(bio_op(bio) == REQ_OP_ZONE_APPEND); BUG_ON(bi_size > *tio->len_ptr); BUG_ON(n_sectors > bi_size); + *tio->len_ptr -= bi_size - n_sectors; bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT; } Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 01/11] block: improve handling of all zones reset operation
On 5/24/21 9:25 PM, Damien Le Moal wrote: SCSI, ZNS and null_blk zoned devices support resetting all zones using a single command (REQ_OP_ZONE_RESET_ALL), as indicated using the device request queue flag QUEUE_FLAG_ZONE_RESETALL. This flag is not set for device mapper targets creating zoned devices. In this case, a user request for resetting all zones of a device is processed in blkdev_zone_mgmt() by issuing a REQ_OP_ZONE_RESET operation for each zone of the device. This leads to different behaviors of the BLKRESETZONE ioctl() depending on the target device support for the reset all operation. E.g. blkzone reset /dev/sdX will reset all zones of a SCSI device using a single command that will ignore conventional, read-only or offline zones. But a dm-linear device including conventional, read-only or offline zones cannot be reset in the same manner as some of the single zone reset operations issued by blkdev_zone_mgmt() will fail. E.g.: blkzone reset /dev/dm-Y blkzone: /dev/dm-0: BLKRESETZONE ioctl failed: Remote I/O error To simplify applications and tools development, unify the behavior of the all-zone reset operation by modifying blkdev_zone_mgmt() to not issue a zone reset operation for conventional, read-only and offline zones, thus mimicking what an actual reset-all device command does on a device supporting REQ_OP_ZONE_RESET_ALL. This emulation is done using the new function blkdev_zone_reset_all_emulated(). The zones needing a reset are identified using a bitmap that is initialized using a zone report. Since empty zones do not need a reset, also ignore these zones. The function blkdev_zone_reset_all() is introduced for block devices natively supporting reset all operations. blkdev_zone_mgmt() is modified to call either function to execute an all zone reset request. Signed-off-by: Damien Le Moal [hch: split into multiple functions] Signed-off-by: Christoph Hellwig --- block/blk-zoned.c | 119 +++--- 1 file changed, 92 insertions(+), 27 deletions(-) diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 250cb76ee615..f47f688b6ea6 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -161,18 +161,89 @@ int blkdev_report_zones(struct block_device *bdev, sector_t sector, } EXPORT_SYMBOL_GPL(blkdev_report_zones); -static inline bool blkdev_allow_reset_all_zones(struct block_device *bdev, - sector_t sector, - sector_t nr_sectors) +static inline unsigned long *blk_alloc_zone_bitmap(int node, + unsigned int nr_zones) { - if (!blk_queue_zone_resetall(bdev_get_queue(bdev))) - return false; + return kcalloc_node(BITS_TO_LONGS(nr_zones), sizeof(unsigned long), + GFP_NOIO, node); +} +static int blk_zone_need_reset_cb(struct blk_zone *zone, unsigned int idx, + void *data) +{ /* -* REQ_OP_ZONE_RESET_ALL can be executed only if the number of sectors -* of the applicable zone range is the entire disk. +* For an all-zones reset, ignore conventional, empty, read-only +* and offline zones. */ - return !sector && nr_sectors == get_capacity(bdev->bd_disk); + switch (zone->cond) { + case BLK_ZONE_COND_NOT_WP: + case BLK_ZONE_COND_EMPTY: + case BLK_ZONE_COND_READONLY: + case BLK_ZONE_COND_OFFLINE: + return 0; + default: + set_bit(idx, (unsigned long *)data); + return 0; + } +} + +static int blkdev_zone_reset_all_emulated(struct block_device *bdev, + gfp_t gfp_mask) +{ + struct request_queue *q = bdev_get_queue(bdev); + sector_t capacity = get_capacity(bdev->bd_disk); + sector_t zone_sectors = blk_queue_zone_sectors(q); + unsigned long *need_reset; + struct bio *bio = NULL; + sector_t sector = 0; + int ret; + + need_reset = blk_alloc_zone_bitmap(q->node, q->nr_zones); + if (!need_reset) + return -ENOMEM; + + ret = bdev->bd_disk->fops->report_zones(bdev->bd_disk, 0, + q->nr_zones, blk_zone_need_reset_cb, + need_reset); + if (ret < 0) + goto out_free_need_reset; + + ret = 0; + while (sector < capacity) { + if (!test_bit(blk_queue_zone_no(q, sector), need_reset)) { + sector += zone_sectors; + continue; + } + + bio = blk_next_bio(bio, 0, gfp_mask); + bio_set_dev(bio, bdev); + bio->bi_opf = REQ_OP_ZONE_RESET | REQ_SYNC; + bio->bi_iter.bi_sector = sector; + sector += zone_sectors; + + /* This may take a while, so be nice to others */ +
Re: [dm-devel] [PATCH v4 07/11] dm: Introduce dm_report_zones()
On 5/24/21 9:25 PM, Damien Le Moal wrote: To simplify the implementation of the report_zones operation of a zoned target, introduce the function dm_report_zones() to set a target mapping start sector in struct dm_report_zones_args and call blkdev_report_zones(). This new function is exported and the report zones callback function dm_report_zones_cb() is not. dm-linear, dm-flakey and dm-crypt are modified to use dm_report_zones(). Signed-off-by: Damien Le Moal Reviewed-by: Johannes Thumshirn Reviewed-by: Hannes Reinecke --- drivers/md/dm-crypt.c | 7 +++ drivers/md/dm-flakey.c| 7 +++ drivers/md/dm-linear.c| 7 +++ drivers/md/dm-zone.c | 23 --- include/linux/device-mapper.h | 3 ++- 5 files changed, 31 insertions(+), 16 deletions(-) diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index b0ab080f2567..f410ceee51d7 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -3138,11 +3138,10 @@ static int crypt_report_zones(struct dm_target *ti, struct dm_report_zones_args *args, unsigned int nr_zones) { struct crypt_config *cc = ti->private; - sector_t sector = cc->start + dm_target_offset(ti, args->next_sector); - args->start = cc->start; - return blkdev_report_zones(cc->dev->bdev, sector, nr_zones, - dm_report_zones_cb, args); + return dm_report_zones(cc->dev->bdev, cc->start, + cc->start + dm_target_offset(ti, args->next_sector), + args, nr_zones); } #else #define crypt_report_zones NULL diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c index b7fee9936f05..5877220c01ed 100644 --- a/drivers/md/dm-flakey.c +++ b/drivers/md/dm-flakey.c @@ -463,11 +463,10 @@ static int flakey_report_zones(struct dm_target *ti, struct dm_report_zones_args *args, unsigned int nr_zones) { struct flakey_c *fc = ti->private; - sector_t sector = flakey_map_sector(ti, args->next_sector); - args->start = fc->start; - return blkdev_report_zones(fc->dev->bdev, sector, nr_zones, - dm_report_zones_cb, args); + return dm_report_zones(fc->dev->bdev, fc->start, + flakey_map_sector(ti, args->next_sector), + args, nr_zones); } #else #define flakey_report_zones NULL diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c index 92db0f5e7f28..c91f1e2e2f65 100644 --- a/drivers/md/dm-linear.c +++ b/drivers/md/dm-linear.c @@ -140,11 +140,10 @@ static int linear_report_zones(struct dm_target *ti, struct dm_report_zones_args *args, unsigned int nr_zones) { struct linear_c *lc = ti->private; - sector_t sector = linear_map_sector(ti, args->next_sector); - args->start = lc->start; - return blkdev_report_zones(lc->dev->bdev, sector, nr_zones, - dm_report_zones_cb, args); + return dm_report_zones(lc->dev->bdev, lc->start, + linear_map_sector(ti, args->next_sector), + args, nr_zones); } #else #define linear_report_zones NULL diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c index 3243c42b7951..b42474043249 100644 --- a/drivers/md/dm-zone.c +++ b/drivers/md/dm-zone.c @@ -56,7 +56,8 @@ int dm_blk_report_zones(struct gendisk *disk, sector_t sector, return ret; } -int dm_report_zones_cb(struct blk_zone *zone, unsigned int idx, void *data) +static int dm_report_zones_cb(struct blk_zone *zone, unsigned int idx, + void *data) { struct dm_report_zones_args *args = data; sector_t sector_diff = args->tgt->begin - args->start; @@ -84,7 +85,24 @@ int dm_report_zones_cb(struct blk_zone *zone, unsigned int idx, void *data) args->next_sector = zone->start + zone->len; return args->orig_cb(zone, args->zone_idx++, args->orig_data); } -EXPORT_SYMBOL_GPL(dm_report_zones_cb); + +/* + * Helper for drivers of zoned targets to implement struct target_type + * report_zones operation. + */ +int dm_report_zones(struct block_device *bdev, sector_t start, sector_t sector, + struct dm_report_zones_args *args, unsigned int nr_zones) +{ + /* +* Set the target mapping start sector first so that +* dm_report_zones_cb() can correctly remap zone information. +*/ + args->start = start; + + return blkdev_report_zones(bdev, sector, nr_zones, + dm_report_zones_cb, args); +} +EXPORT_SYMBOL_GPL(dm_report_zones); void dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q) { @@ -99,4 +117,3 @@ void dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q) WARN_ON_ONCE(queue_is_mq(q)); q->nr_zones = blkdev_nr_zones(t->md->disk); } - diff
Re: [dm-devel] [PATCH v4 11/11] dm crypt: Fix zoned block device support
On 5/24/21 9:25 PM, Damien Le Moal wrote: Zone append BIOs (REQ_OP_ZONE_APPEND) always specify the start sector of the zone to be written instead of the actual sector location to write. The write location is determined by the device and returned to the host upon completion of the operation. This interface, while simple and efficient for writing into sequential zones of a zoned block device, is incompatible with the use of sector values to calculate a cypher block IV. All data written in a zone end up using the same IV values corresponding to the first sectors of the zone, but read operation will specify any sector within the zone resulting in an IV mismatch between encryption and decryption. To solve this problem, report to DM core that zone append operations are not supported. This result in the zone append operations being emulated using regular write operations. Reported-by: Shin'ichiro Kawasaki Signed-off-by: Damien Le Moal Reviewed-by: Hannes Reinecke --- drivers/md/dm-crypt.c | 24 +++- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index f410ceee51d7..50f4cbd600d5 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -3280,14 +3280,28 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) } cc->start = tmpll; - /* -* For zoned block devices, we need to preserve the issuer write -* ordering. To do so, disable write workqueues and force inline -* encryption completion. -*/ if (bdev_is_zoned(cc->dev->bdev)) { + /* +* For zoned block devices, we need to preserve the issuer write +* ordering. To do so, disable write workqueues and force inline +* encryption completion. +*/ set_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, >flags); set_bit(DM_CRYPT_WRITE_INLINE, >flags); + + /* +* All zone append writes to a zone of a zoned block device will +* have the same BIO sector, the start of the zone. When the +* cypher IV mode uses sector values, all data targeting a +* zone will be encrypted using the first sector numbers of the +* zone. This will not result in write errors but will +* cause most reads to fail as reads will use the sector values +* for the actual data locations, resulting in IV mismatch. +* To avoid this problem, ask DM core to emulate zone append +* operations with regular writes. +*/ + DMDEBUG("Zone append operations will be emulated"); + ti->emulate_zone_append = true; } if (crypt_integrity_aead(cc) || cc->integrity_iv_size) { Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 06/11] dm: move zone related code to dm-zone.c
On 5/24/21 9:25 PM, Damien Le Moal wrote: Move core and table code used for zoned targets and conditionally defined with #ifdef CONFIG_BLK_DEV_ZONED to the new file dm-zone.c. This file is conditionally compiled depending on CONFIG_BLK_DEV_ZONED. The small helper dm_set_zones_restrictions() is introduced to initialize a mapped device request queue zone attributes in dm_table_set_restrictions(). Signed-off-by: Damien Le Moal Reviewed-by: Johannes Thumshirn Reviewed-by: Hannes Reinecke --- drivers/md/Makefile | 4 ++ drivers/md/dm-table.c | 14 ++ drivers/md/dm-zone.c | 102 ++ drivers/md/dm.c | 78 drivers/md/dm.h | 11 + 5 files changed, 120 insertions(+), 89 deletions(-) create mode 100644 drivers/md/dm-zone.c diff --git a/drivers/md/Makefile b/drivers/md/Makefile index ef7ddc27685c..a74aaf8b1445 100644 --- a/drivers/md/Makefile +++ b/drivers/md/Makefile @@ -92,6 +92,10 @@ ifeq ($(CONFIG_DM_UEVENT),y) dm-mod-objs += dm-uevent.o endif +ifeq ($(CONFIG_BLK_DEV_ZONED),y) +dm-mod-objs+= dm-zone.o +endif + ifeq ($(CONFIG_DM_VERITY_FEC),y) dm-verity-objs+= dm-verity-fec.o endif diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 21fd9cd4da32..dd9f648ab598 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -2064,17 +2064,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, dm_table_any_dev_attr(t, device_is_not_random, NULL)) blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q); - /* -* For a zoned target, the number of zones should be updated for the -* correct value to be exposed in sysfs queue/nr_zones. For a BIO based -* target, this is all that is needed. -*/ -#ifdef CONFIG_BLK_DEV_ZONED - if (blk_queue_is_zoned(q)) { - WARN_ON_ONCE(queue_is_mq(q)); - q->nr_zones = blkdev_nr_zones(t->md->disk); - } -#endif + /* For a zoned target, setup the zones related queue attributes */ + if (blk_queue_is_zoned(q)) + dm_set_zones_restrictions(t, q); dm_update_keyslot_manager(q, t); blk_queue_update_readahead(q); diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c new file mode 100644 index ..3243c42b7951 --- /dev/null +++ b/drivers/md/dm-zone.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Western Digital Corporation or its affiliates. + */ + +#include + +#include "dm-core.h" + +/* + * User facing dm device block device report zone operation. This calls the + * report_zones operation for each target of a device table. This operation is + * generally implemented by targets using dm_report_zones(). + */ +int dm_blk_report_zones(struct gendisk *disk, sector_t sector, + unsigned int nr_zones, report_zones_cb cb, void *data) +{ + struct mapped_device *md = disk->private_data; + struct dm_table *map; + int srcu_idx, ret; + struct dm_report_zones_args args = { + .next_sector = sector, + .orig_data = data, + .orig_cb = cb, + }; + + if (dm_suspended_md(md)) + return -EAGAIN; + + map = dm_get_live_table(md, _idx); + if (!map) { + ret = -EIO; + goto out; + } + + do { + struct dm_target *tgt; + + tgt = dm_table_find_target(map, args.next_sector); + if (WARN_ON_ONCE(!tgt->type->report_zones)) { + ret = -EIO; + goto out; + } + + args.tgt = tgt; + ret = tgt->type->report_zones(tgt, , + nr_zones - args.zone_idx); + if (ret < 0) + goto out; + } while (args.zone_idx < nr_zones && +args.next_sector < get_capacity(disk)); + + ret = args.zone_idx; +out: + dm_put_live_table(md, srcu_idx); + return ret; +} + +int dm_report_zones_cb(struct blk_zone *zone, unsigned int idx, void *data) +{ + struct dm_report_zones_args *args = data; + sector_t sector_diff = args->tgt->begin - args->start; + + /* +* Ignore zones beyond the target range. +*/ + if (zone->start >= args->start + args->tgt->len) + return 0; + + /* +* Remap the start sector and write pointer position of the zone +* to match its position in the target range. +*/ + zone->start += sector_diff; + if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) { + if (zone->cond == BLK_ZONE_COND_FULL) + zone->wp = zone->start + zone->len; + else if (zone->cond == BLK_ZONE_COND_EMPTY) + zone->wp =
Re: [dm-devel] [PATCH v4 08/11] dm: Forbid requeue of writes to zones
On 5/24/21 9:25 PM, Damien Le Moal wrote: A target map method requesting the requeue of a bio with DM_MAPIO_REQUEUE or completing it with DM_ENDIO_REQUEUE can cause unaligned write errors if the bio is a write operation targeting a sequential zone. If a zoned target request such a requeue, warn about it and kill the IO. The function dm_is_zone_write() is introduced to detect write operations to zoned targets. This change does not affect the target drivers supporting zoned devices and exposing a zoned device, namely dm-crypt, dm-linear and dm-flakey as none of these targets ever request a requeue. Signed-off-by: Damien Le Moal Reviewed-by: Hannes Reinecke --- drivers/md/dm-zone.c | 17 + drivers/md/dm.c | 18 +++--- drivers/md/dm.h | 5 + 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c index b42474043249..edc3bbb45637 100644 --- a/drivers/md/dm-zone.c +++ b/drivers/md/dm-zone.c @@ -104,6 +104,23 @@ int dm_report_zones(struct block_device *bdev, sector_t start, sector_t sector, } EXPORT_SYMBOL_GPL(dm_report_zones); +bool dm_is_zone_write(struct mapped_device *md, struct bio *bio) +{ + struct request_queue *q = md->queue; + + if (!blk_queue_is_zoned(q)) + return false; + + switch (bio_op(bio)) { + case REQ_OP_WRITE_ZEROES: + case REQ_OP_WRITE_SAME: + case REQ_OP_WRITE: + return !op_is_flush(bio->bi_opf) && bio_sectors(bio); + default: + return false; + } +} + void dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q) { if (!blk_queue_is_zoned(q)) diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 45d2dc2ee844..4426019a89cc 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -846,11 +846,15 @@ static void dec_pending(struct dm_io *io, blk_status_t error) * Target requested pushing back the I/O. */ spin_lock_irqsave(>deferred_lock, flags); - if (__noflush_suspending(md)) + if (__noflush_suspending(md) && + !WARN_ON_ONCE(dm_is_zone_write(md, bio))) /* NOTE early return due to BLK_STS_DM_REQUEUE below */ bio_list_add_head(>deferred, io->orig_bio); else - /* noflush suspend was interrupted. */ + /* +* noflush suspend was interrupted or this is +* a write to a zoned target. +*/ io->status = BLK_STS_IOERR; spin_unlock_irqrestore(>deferred_lock, flags); } @@ -947,7 +951,15 @@ static void clone_endio(struct bio *bio) int r = endio(tio->ti, bio, ); switch (r) { case DM_ENDIO_REQUEUE: - error = BLK_STS_DM_REQUEUE; + /* +* Requeuing writes to a sequential zone of a zoned +* target will break the sequential write pattern: +* fail such IO. +*/ + if (WARN_ON_ONCE(dm_is_zone_write(md, bio))) + error = BLK_STS_IOERR; + else + error = BLK_STS_DM_REQUEUE; fallthrough; case DM_ENDIO_DONE: break; diff --git a/drivers/md/dm.h b/drivers/md/dm.h index fdf1536a4b62..39c243258e24 100644 --- a/drivers/md/dm.h +++ b/drivers/md/dm.h @@ -107,8 +107,13 @@ void dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q); #ifdef CONFIG_BLK_DEV_ZONED int dm_blk_report_zones(struct gendisk *disk, sector_t sector, unsigned int nr_zones, report_zones_cb cb, void *data); +bool dm_is_zone_write(struct mapped_device *md, struct bio *bio); #else #define dm_blk_report_zones NULL +static inline bool dm_is_zone_write(struct mapped_device *md, struct bio *bio) +{ + return false; +} #endif /*- Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 05/11] dm: cleanup device_area_is_invalid()
On 5/24/21 9:25 PM, Damien Le Moal wrote: In device_area_is_invalid(), use bdev_is_zoned() instead of open coding the test on the zoned model returned by bdev_zoned_model(). Signed-off-by: Damien Le Moal Reviewed-by: Johannes Thumshirn Reviewed-by: Hannes Reinecke --- drivers/md/dm-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index ee47a332b462..21fd9cd4da32 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -249,7 +249,7 @@ static int device_area_is_invalid(struct dm_target *ti, struct dm_dev *dev, * If the target is mapped to zoned block device(s), check * that the zones are not partially mapped. */ - if (bdev_zoned_model(bdev) != BLK_ZONED_NONE) { + if (bdev_is_zoned(bdev)) { unsigned int zone_sectors = bdev_zone_sectors(bdev); if (start & (zone_sectors - 1)) { Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 09/11] dm: rearrange core declarations
On 5/24/21 9:25 PM, Damien Le Moal wrote: Move the definitions of struct dm_target_io, struct dm_io and of the bits of the flags field of struct mapped_device from dm.c to dm-core.h to make them usable from dm-zone.c. For the same reason, declare dec_pending() in dm-core.h after renaming it to dm_io_dec_pending(). And for symmetry of the function names, introduce the inline helper dm_io_inc_pending() instead of directly using atomic_inc() calls. Signed-off-by: Damien Le Moal Reviewed-by: Hannes Reinecke --- drivers/md/dm-core.h | 52 ++ drivers/md/dm.c | 59 ++-- 2 files changed, 59 insertions(+), 52 deletions(-) diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h index 5953ff2bd260..cfabc1c91f9f 100644 --- a/drivers/md/dm-core.h +++ b/drivers/md/dm-core.h @@ -116,6 +116,19 @@ struct mapped_device { struct srcu_struct io_barrier; }; +/* + * Bits for the flags field of struct mapped_device. + */ +#define DMF_BLOCK_IO_FOR_SUSPEND 0 +#define DMF_SUSPENDED 1 +#define DMF_FROZEN 2 +#define DMF_FREEING 3 +#define DMF_DELETING 4 +#define DMF_NOFLUSH_SUSPENDING 5 +#define DMF_DEFERRED_REMOVE 6 +#define DMF_SUSPENDED_INTERNALLY 7 +#define DMF_POST_SUSPENDING 8 + void disable_discard(struct mapped_device *md); void disable_write_same(struct mapped_device *md); void disable_write_zeroes(struct mapped_device *md); @@ -173,6 +186,45 @@ struct dm_table { #endif }; +/* + * One of these is allocated per clone bio. + */ +#define DM_TIO_MAGIC 7282014 +struct dm_target_io { + unsigned int magic; + struct dm_io *io; + struct dm_target *ti; + unsigned int target_bio_nr; + unsigned int *len_ptr; + bool inside_dm_io; + struct bio clone; +}; + +/* + * One of these is allocated per original bio. + * It contains the first clone used for that original. + */ +#define DM_IO_MAGIC 5191977 +struct dm_io { + unsigned int magic; + struct mapped_device *md; + blk_status_t status; + atomic_t io_count; + struct bio *orig_bio; + unsigned long start_time; + spinlock_t endio_lock; + struct dm_stats_aux stats_aux; + /* last member of dm_target_io is 'struct bio' */ + struct dm_target_io tio; +}; + +static inline void dm_io_inc_pending(struct dm_io *io) +{ + atomic_inc(>io_count); +} + +void dm_io_dec_pending(struct dm_io *io, blk_status_t error); + static inline struct completion *dm_get_completion_from_kobject(struct kobject *kobj) { return _of(kobj, struct dm_kobject_holder, kobj)->completion; diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 4426019a89cc..563504163b74 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -74,38 +74,6 @@ struct clone_info { unsigned sector_count; }; -/* - * One of these is allocated per clone bio. - */ -#define DM_TIO_MAGIC 7282014 -struct dm_target_io { - unsigned magic; - struct dm_io *io; - struct dm_target *ti; - unsigned target_bio_nr; - unsigned *len_ptr; - bool inside_dm_io; - struct bio clone; -}; - -/* - * One of these is allocated per original bio. - * It contains the first clone used for that original. - */ -#define DM_IO_MAGIC 5191977 -struct dm_io { - unsigned magic; - struct mapped_device *md; - blk_status_t status; - atomic_t io_count; - struct bio *orig_bio; - unsigned long start_time; - spinlock_t endio_lock; - struct dm_stats_aux stats_aux; - /* last member of dm_target_io is 'struct bio' */ - struct dm_target_io tio; -}; - #define DM_TARGET_IO_BIO_OFFSET (offsetof(struct dm_target_io, clone)) #define DM_IO_BIO_OFFSET \ (offsetof(struct dm_target_io, clone) + offsetof(struct dm_io, tio)) @@ -137,19 +105,6 @@ EXPORT_SYMBOL_GPL(dm_bio_get_target_bio_nr); #define MINOR_ALLOCED ((void *)-1) -/* - * Bits for the md->flags field. - */ -#define DMF_BLOCK_IO_FOR_SUSPEND 0 -#define DMF_SUSPENDED 1 -#define DMF_FROZEN 2 -#define DMF_FREEING 3 -#define DMF_DELETING 4 -#define DMF_NOFLUSH_SUSPENDING 5 -#define DMF_DEFERRED_REMOVE 6 -#define DMF_SUSPENDED_INTERNALLY 7 -#define DMF_POST_SUSPENDING 8 - #define DM_NUMA_NODE NUMA_NO_NODE static int dm_numa_node = DM_NUMA_NODE; @@ -825,7 +780,7 @@ static int __noflush_suspending(struct mapped_device *md) * Decrements the number of outstanding ios that a bio has been * cloned into, completing the original io if necc. */ -static void dec_pending(struct dm_io *io, blk_status_t error) +void dm_io_dec_pending(struct dm_io *io, blk_status_t error) { unsigned long flags; blk_status_t io_error; @@ -978,7 +933,7 @@ static void clone_endio(struct bio *bio) } free_tio(tio); - dec_pending(io, error); + dm_io_dec_pending(io, error); } /* @@ -1247,7 +1202,7 @@ static blk_qc_t __map_bio(struct dm_target_io *tio) *
Re: [dm-devel] simplify gendisk and request_queue allocation for bio based drivers
On Fri, 21 May 2021 at 07:51, Christoph Hellwig wrote: > > Hi all, > > this series is the first part of cleaning up lifetimes and allocation of > the gendisk and request_queue structure. It adds a new interface to > allocate the disk and queue together for bio based drivers, and a helper > for cleanup/free them when a driver is unloaded or a device is removed. May I ask what else you have in the pipe for the next steps? The reason why I ask is that I am looking into some issues related to lifecycle problems of gendisk/mmc, typically triggered at SD/MMC card removal. > > Together this removes the need to treat the gendisk and request_queue > as separate entities for bio based drivers. > > Diffstat: > arch/m68k/emu/nfblock.c | 20 +--- > arch/xtensa/platforms/iss/simdisk.c | 29 +-- > block/blk-core.c|1 > block/blk.h |6 - > block/genhd.c | 149 > +++- > block/partitions/core.c | 19 ++-- > drivers/block/brd.c | 94 +++--- > drivers/block/drbd/drbd_main.c | 23 + > drivers/block/n64cart.c |8 - > drivers/block/null_blk/main.c | 38 - > drivers/block/pktcdvd.c | 11 -- > drivers/block/ps3vram.c | 31 +-- > drivers/block/rsxx/dev.c| 39 +++-- > drivers/block/rsxx/rsxx_priv.h |1 > drivers/block/zram/zram_drv.c | 19 > drivers/lightnvm/core.c | 24 + > drivers/md/bcache/super.c | 15 --- > drivers/md/dm.c | 16 +-- > drivers/md/md.c | 25 ++ > drivers/memstick/core/ms_block.c|1 > drivers/nvdimm/blk.c| 27 +- > drivers/nvdimm/btt.c| 25 +- > drivers/nvdimm/btt.h|2 > drivers/nvdimm/pmem.c | 17 +--- > drivers/nvme/host/core.c|1 > drivers/nvme/host/multipath.c | 46 +++ > drivers/s390/block/dcssblk.c| 26 +- > drivers/s390/block/xpram.c | 26 ++ > include/linux/blkdev.h |1 > include/linux/genhd.h | 23 + > 30 files changed, 297 insertions(+), 466 deletions(-) This looks like a nice cleanup to me. Feel free to add, for the series: Reviewed-by: Ulf Hansson Kind regards Uffe -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] simplify gendisk and request_queue allocation for bio based drivers
On Wed, 26 May 2021 at 06:49, Christoph Hellwig wrote: > > On Wed, May 26, 2021 at 12:41:37AM +0200, Ulf Hansson wrote: > > On Fri, 21 May 2021 at 07:51, Christoph Hellwig wrote: > > > > > > Hi all, > > > > > > this series is the first part of cleaning up lifetimes and allocation of > > > the gendisk and request_queue structure. It adds a new interface to > > > allocate the disk and queue together for bio based drivers, and a helper > > > for cleanup/free them when a driver is unloaded or a device is removed. > > > > May I ask what else you have in the pipe for the next steps? > > > > The reason why I ask is that I am looking into some issues related to > > lifecycle problems of gendisk/mmc, typically triggered at SD/MMC card > > removal. > > In the short run not much more than superficial cleanups. Eventually > I want bio based drivers to not require a separate request_queue, leaving > that purely as a data structure for blk-mq based drivers. But it will > take a while until we get there, so it should not block any fixes. Alright, thanks for clarifying. > > For hot unplug handling it might be worth to take a look at nvme, as it > is tested a lot for that case. Okay, thanks for the hint. Kind regards Uffe -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 03/11] block: introduce BIO_ZONE_WRITE_LOCKED bio flag
On 5/24/21 9:25 PM, Damien Le Moal wrote: Introduce the BIO flag BIO_ZONE_WRITE_LOCKED to indicate that a BIO owns the write lock of the zone it is targeting. This is the counterpart of the struct request flag RQF_ZONE_WRITE_LOCKED. This new BIO flag is reserved for now for zone write locking control for device mapper targets exposing a zoned block device. Since in this case, the lock flag must not be propagated to the struct request that will be used to process the BIO, a BIO private flag is used rather than changing the RQF_ZONE_WRITE_LOCKED request flag into a common REQ_XXX flag that could be used for both BIO and request. This avoids conflicts down the stack with the block IO scheduler zone write locking (in mq-deadline). Signed-off-by: Damien Le Moal Reviewed-by: Christoph Hellwig Reviewed-by: Hannes Reinecke Reviewed-by: Chaitanya Kulkarni --- include/linux/blk_types.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index db026b6ec15a..e5cf12f102a2 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -304,6 +304,7 @@ enum { BIO_CGROUP_ACCT,/* has been accounted to a cgroup */ BIO_TRACKED,/* set if bio goes through the rq_qos path */ BIO_REMAPPED, + BIO_ZONE_WRITE_LOCKED, /* Owns a zoned device zone write lock */ BIO_FLAG_LAST }; Reviewed-by: Himanshu Madhani -- Himanshu MadhaniOracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 10/11] dm: introduce zone append emulation
On 5/24/21 9:25 PM, Damien Le Moal wrote: For zoned targets that cannot support zone append operations, implement an emulation using regular write operations. If the original BIO submitted by the user is a zone append operation, change its clone into a regular write operation directed at the target zone write pointer position. To do so, an array of write pointer offsets (write pointer position relative to the start of a zone) is added to struct mapped_device. All operations that modify a sequential zone write pointer (writes, zone reset, zone finish and zone append) are intersepted in __map_bio() and processed using the new functions dm_zone_map_bio(). Detection of the target ability to natively support zone append operations is done from dm_table_set_restrictions() by calling the function dm_set_zones_restrictions(). A target that does not support zone append operation, either by explicitly declaring it using the new struct dm_target field zone_append_not_supported, or because the device table contains a non-zoned device, has its mapped device marked with the new flag DMF_ZONE_APPEND_EMULATED. The helper function dm_emulate_zone_append() is introduced to test a mapped device for this new flag. Atomicity of the zones write pointer tracking and updates is done using a zone write locking mechanism based on a bitmap. This is similar to the block layer method but based on BIOs rather than struct request. A zone write lock is taken in dm_zone_map_bio() for any clone BIO with an operation type that changes the BIO target zone write pointer position. The zone write lock is released if the clone BIO is failed before submission or when dm_zone_endio() is called when the clone BIO completes. The zone write lock bitmap of the mapped device, together with a bitmap indicating zone types (conv_zones_bitmap) and the write pointer offset array (zwp_offset) are allocated and initialized with a full device zone report in dm_set_zones_restrictions() using the function dm_revalidate_zones(). For failed operations that may have modified a zone write pointer, the zone write pointer offset is marked as invalid in dm_zone_endio(). Zones with an invalid write pointer offset are checked and the write pointer updated using an internal report zone operation when the faulty zone is accessed again by the user. All functions added for this emulation have a minimal overhead for zoned targets natively supporting zone append operations. Regular device targets are also not affected. The added code also does not impact builds with CONFIG_BLK_DEV_ZONED disabled by stubbing out all dm zone related functions. Signed-off-by: Damien Le Moal --- drivers/md/dm-core.h | 13 + drivers/md/dm-table.c | 19 +- drivers/md/dm-zone.c | 580 -- drivers/md/dm.c | 38 ++- drivers/md/dm.h | 16 +- include/linux/device-mapper.h | 6 + 6 files changed, 618 insertions(+), 54 deletions(-) diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h index cfabc1c91f9f..edc1553c4eea 100644 --- a/drivers/md/dm-core.h +++ b/drivers/md/dm-core.h @@ -114,6 +114,11 @@ struct mapped_device { bool init_tio_pdu:1; struct srcu_struct io_barrier; + +#ifdef CONFIG_BLK_DEV_ZONED + unsigned int nr_zones; + unsigned int *zwp_offset; +#endif }; /* @@ -128,6 +133,7 @@ struct mapped_device { #define DMF_DEFERRED_REMOVE 6 #define DMF_SUSPENDED_INTERNALLY 7 #define DMF_POST_SUSPENDING 8 +#define DMF_EMULATE_ZONE_APPEND 9 void disable_discard(struct mapped_device *md); void disable_write_same(struct mapped_device *md); @@ -143,6 +149,13 @@ static inline struct dm_stats *dm_get_stats(struct mapped_device *md) return >stats; } +static inline bool dm_emulate_zone_append(struct mapped_device *md) +{ + if (blk_queue_is_zoned(md->queue)) + return test_bit(DMF_EMULATE_ZONE_APPEND, >flags); + return false; +} + #define DM_TABLE_MAX_DEPTH 16 struct dm_table { diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index dd9f648ab598..21fdccfb16cf 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -1981,11 +1981,12 @@ static int device_requires_stable_pages(struct dm_target *ti, return blk_queue_stable_writes(q); } -void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, - struct queue_limits *limits) +int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, + struct queue_limits *limits) { bool wc = false, fua = false; int page_size = PAGE_SIZE; + int r; /* * Copy table's limits to the DM device's request_queue @@ -2064,12 +2065,20 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, dm_table_any_dev_attr(t, device_is_not_random, NULL))
Re: [dm-devel] [PATCH][next] dm btree: Fix potential read of array with negative index i
ack On Fri, May 21, 2021 at 11:52 AM Colin King wrote: > From: Colin Ian King > > The call to lower_bound can return -1 if the key is not found > with the bsearch, leading to a negative index access into > array node->keys[]. Ensure this cannot occur by checking for > a negative index before reading from the array. > > Addresses-Coverity: ("Negative array index read") > Fixes: d69e2e7e28bd ("dm btree: improve btree residency") > Signed-off-by: Colin Ian King > --- > drivers/md/persistent-data/dm-btree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/persistent-data/dm-btree.c > b/drivers/md/persistent-data/dm-btree.c > index b8d21b6e2953..266deaea5eea 100644 > --- a/drivers/md/persistent-data/dm-btree.c > +++ b/drivers/md/persistent-data/dm-btree.c > @@ -1048,7 +1048,7 @@ static bool contains_key(struct btree_node *node, > uint64_t key) > { > int i = lower_bound(node, key); > > - if (le64_to_cpu(node->keys[i]) == key) > + if (i >= 0 && le64_to_cpu(node->keys[i]) == key) > return true; > > return false; > -- > 2.31.1 > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v5 10/11] dm: introduce zone append emulation
On 5/25/21 11:25 PM, Damien Le Moal wrote: For zoned targets that cannot support zone append operations, implement an emulation using regular write operations. If the original BIO submitted by the user is a zone append operation, change its clone into a regular write operation directed at the target zone write pointer position. To do so, an array of write pointer offsets (write pointer position relative to the start of a zone) is added to struct mapped_device. All operations that modify a sequential zone write pointer (writes, zone reset, zone finish and zone append) are intersepted in __map_bio() and processed using the new functions dm_zone_map_bio(). Detection of the target ability to natively support zone append operations is done from dm_table_set_restrictions() by calling the function dm_set_zones_restrictions(). A target that does not support zone append operation, either by explicitly declaring it using the new struct dm_target field zone_append_not_supported, or because the device table contains a non-zoned device, has its mapped device marked with the new flag DMF_ZONE_APPEND_EMULATED. The helper function dm_emulate_zone_append() is introduced to test a mapped device for this new flag. Atomicity of the zones write pointer tracking and updates is done using a zone write locking mechanism based on a bitmap. This is similar to the block layer method but based on BIOs rather than struct request. A zone write lock is taken in dm_zone_map_bio() for any clone BIO with an operation type that changes the BIO target zone write pointer position. The zone write lock is released if the clone BIO is failed before submission or when dm_zone_endio() is called when the clone BIO completes. The zone write lock bitmap of the mapped device, together with a bitmap indicating zone types (conv_zones_bitmap) and the write pointer offset array (zwp_offset) are allocated and initialized with a full device zone report in dm_set_zones_restrictions() using the function dm_revalidate_zones(). For failed operations that may have modified a zone write pointer, the zone write pointer offset is marked as invalid in dm_zone_endio(). Zones with an invalid write pointer offset are checked and the write pointer updated using an internal report zone operation when the faulty zone is accessed again by the user. All functions added for this emulation have a minimal overhead for zoned targets natively supporting zone append operations. Regular device targets are also not affected. The added code also does not impact builds with CONFIG_BLK_DEV_ZONED disabled by stubbing out all dm zone related functions. Signed-off-by: Damien Le Moal Reviewed-by: Himanshu Madhani --- drivers/md/dm-core.h | 13 + drivers/md/dm-table.c | 19 +- drivers/md/dm-zone.c | 580 -- drivers/md/dm.c | 38 ++- drivers/md/dm.h | 16 +- include/linux/device-mapper.h | 6 + 6 files changed, 618 insertions(+), 54 deletions(-) Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeKernel Storage Architect h...@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v5 01/11] block: improve handling of all zones reset operation
On 5/25/21 11:24 PM, Damien Le Moal wrote: SCSI, ZNS and null_blk zoned devices support resetting all zones using a single command (REQ_OP_ZONE_RESET_ALL), as indicated using the device request queue flag QUEUE_FLAG_ZONE_RESETALL. This flag is not set for device mapper targets creating zoned devices. In this case, a user request for resetting all zones of a device is processed in blkdev_zone_mgmt() by issuing a REQ_OP_ZONE_RESET operation for each zone of the device. This leads to different behaviors of the BLKRESETZONE ioctl() depending on the target device support for the reset all operation. E.g. blkzone reset /dev/sdX will reset all zones of a SCSI device using a single command that will ignore conventional, read-only or offline zones. But a dm-linear device including conventional, read-only or offline zones cannot be reset in the same manner as some of the single zone reset operations issued by blkdev_zone_mgmt() will fail. E.g.: blkzone reset /dev/dm-Y blkzone: /dev/dm-0: BLKRESETZONE ioctl failed: Remote I/O error To simplify applications and tools development, unify the behavior of the all-zone reset operation by modifying blkdev_zone_mgmt() to not issue a zone reset operation for conventional, read-only and offline zones, thus mimicking what an actual reset-all device command does on a device supporting REQ_OP_ZONE_RESET_ALL. This emulation is done using the new function blkdev_zone_reset_all_emulated(). The zones needing a reset are identified using a bitmap that is initialized using a zone report. Since empty zones do not need a reset, also ignore these zones. The function blkdev_zone_reset_all() is introduced for block devices natively supporting reset all operations. blkdev_zone_mgmt() is modified to call either function to execute an all zone reset request. Signed-off-by: Damien Le Moal [hch: split into multiple functions] Signed-off-by: Christoph Hellwig Reviewed-by: Chaitanya Kulkarni --- block/blk-zoned.c | 119 +++--- 1 file changed, 92 insertions(+), 27 deletions(-) Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeKernel Storage Architect h...@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel