On 2019/11/07 9:19, Dmitry Fomichev wrote: > Commit 75d66ffb48efb3 added backing device health checks and as a part > of these checks, check_events() block ops template call is invoked > in dm-zoned mapping path as well as in reclaim and flush path. Calling > check_events() with ATA or SCSI backing devices introduces a blocking > scsi_test_unit_ready() call being made in sd_check_events(). > Even though the overhead of calling scsi_test_unit_ready() is small > for ATA zoned devices, it is much larger for SCSI and it affects > performance in a very negative way. > > This commit fixes this performance regression by executing > check_events() only in case of any I/O errors. The function > dmz_bdev_is_dying() is modified to call only blk_queue_dying(), > while calls to check_events() are made in a new helper function, > dmz_check_bdev().
Looks good to me. Reviewed-by: Damien Le Moal <[email protected]> > > Cc: [email protected] > Fixes: 75d66ffb48efb3 ("dm zoned: properly handle backing device failure") > Signed-off-by: Dmitry Fomichev <[email protected]> > --- > drivers/md/dm-zoned-metadata.c | 29 +++++++++++------- > drivers/md/dm-zoned-reclaim.c | 8 ++--- > drivers/md/dm-zoned-target.c | 54 ++++++++++++++++++++++++---------- > drivers/md/dm-zoned.h | 2 ++ > 4 files changed, 61 insertions(+), 32 deletions(-) > > diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c > index 595a73110e17..ac1179ca80d9 100644 > --- a/drivers/md/dm-zoned-metadata.c > +++ b/drivers/md/dm-zoned-metadata.c > @@ -554,6 +554,7 @@ static struct dmz_mblock *dmz_get_mblock(struct > dmz_metadata *zmd, > TASK_UNINTERRUPTIBLE); > if (test_bit(DMZ_META_ERROR, &mblk->state)) { > dmz_release_mblock(zmd, mblk); > + dmz_check_bdev(zmd->dev); > return ERR_PTR(-EIO); > } > > @@ -625,6 +626,8 @@ static int dmz_rdwr_block(struct dmz_metadata *zmd, int > op, sector_t block, > ret = submit_bio_wait(bio); > bio_put(bio); > > + if (ret) > + dmz_check_bdev(zmd->dev); > return ret; > } > > @@ -691,6 +694,7 @@ static int dmz_write_dirty_mblocks(struct dmz_metadata > *zmd, > TASK_UNINTERRUPTIBLE); > if (test_bit(DMZ_META_ERROR, &mblk->state)) { > clear_bit(DMZ_META_ERROR, &mblk->state); > + dmz_check_bdev(zmd->dev); > ret = -EIO; > } > nr_mblks_submitted--; > @@ -768,7 +772,7 @@ int dmz_flush_metadata(struct dmz_metadata *zmd) > /* If there are no dirty metadata blocks, just flush the device cache */ > if (list_empty(&write_list)) { > ret = blkdev_issue_flush(zmd->dev->bdev, GFP_NOIO, NULL); > - goto out; > + goto err; > } > > /* > @@ -778,7 +782,7 @@ int dmz_flush_metadata(struct dmz_metadata *zmd) > */ > ret = dmz_log_dirty_mblocks(zmd, &write_list); > if (ret) > - goto out; > + goto err; > > /* > * The log is on disk. It is now safe to update in place > @@ -786,11 +790,11 @@ int dmz_flush_metadata(struct dmz_metadata *zmd) > */ > ret = dmz_write_dirty_mblocks(zmd, &write_list, zmd->mblk_primary); > if (ret) > - goto out; > + goto err; > > ret = dmz_write_sb(zmd, zmd->mblk_primary); > if (ret) > - goto out; > + goto err; > > while (!list_empty(&write_list)) { > mblk = list_first_entry(&write_list, struct dmz_mblock, link); > @@ -805,16 +809,20 @@ int dmz_flush_metadata(struct dmz_metadata *zmd) > > zmd->sb_gen++; > out: > - if (ret && !list_empty(&write_list)) { > - spin_lock(&zmd->mblk_lock); > - list_splice(&write_list, &zmd->mblk_dirty_list); > - spin_unlock(&zmd->mblk_lock); > - } > - > dmz_unlock_flush(zmd); > up_write(&zmd->mblk_sem); > > return ret; > + > +err: > + if (!list_empty(&write_list)) { > + spin_lock(&zmd->mblk_lock); > + list_splice(&write_list, &zmd->mblk_dirty_list); > + spin_unlock(&zmd->mblk_lock); > + } > + if (!dmz_check_bdev(zmd->dev)) > + ret = -EIO; > + goto out; > } > > /* > @@ -1244,6 +1252,7 @@ static int dmz_update_zone(struct dmz_metadata *zmd, > struct dm_zone *zone) > if (ret) { > dmz_dev_err(zmd->dev, "Get zone %u report failed", > dmz_id(zmd, zone)); > + dmz_check_bdev(zmd->dev); > return ret; > } > > diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c > index d240d7ca8a8a..e7ace908a9b7 100644 > --- a/drivers/md/dm-zoned-reclaim.c > +++ b/drivers/md/dm-zoned-reclaim.c > @@ -82,6 +82,7 @@ static int dmz_reclaim_align_wp(struct dmz_reclaim *zrc, > struct dm_zone *zone, > "Align zone %u wp %llu to %llu (wp+%u) blocks > failed %d", > dmz_id(zmd, zone), (unsigned long long)wp_block, > (unsigned long long)block, nr_blocks, ret); > + dmz_check_bdev(zrc->dev); > return ret; > } > > @@ -489,12 +490,7 @@ static void dmz_reclaim_work(struct work_struct *work) > ret = dmz_do_reclaim(zrc); > if (ret) { > dmz_dev_debug(zrc->dev, "Reclaim error %d\n", ret); > - if (ret == -EIO) > - /* > - * LLD might be performing some error handling sequence > - * at the underlying device. To not interfere, do not > - * attempt to schedule the next reclaim run immediately. > - */ > + if (!dmz_check_bdev(zrc->dev)) > return; > } > > diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c > index 09f5a63e0803..e0b809bb4885 100644 > --- a/drivers/md/dm-zoned-target.c > +++ b/drivers/md/dm-zoned-target.c > @@ -85,6 +85,8 @@ static inline void dmz_bio_endio(struct bio *bio, > blk_status_t status) > > if (status != BLK_STS_OK && bio->bi_status == BLK_STS_OK) > bio->bi_status = status; > + if (bio->bi_status != BLK_STS_OK) > + bioctx->target->dev->flags |= DMZ_CHECK_BDEV; > > if (refcount_dec_and_test(&bioctx->ref)) { > struct dm_zone *zone = bioctx->zone; > @@ -570,31 +572,51 @@ static int dmz_queue_chunk_work(struct dmz_target *dmz, > struct bio *bio) > } > > /* > - * Check the backing device availability. If it's on the way out, > + * Check if the backing device is being removed. If it's on the way out, > * start failing I/O. Reclaim and metadata components also call this > * function to cleanly abort operation in the event of such failure. > */ > bool dmz_bdev_is_dying(struct dmz_dev *dmz_dev) > { > - struct gendisk *disk; > + if (dmz_dev->flags & DMZ_BDEV_DYING) > + return true; > > - if (!(dmz_dev->flags & DMZ_BDEV_DYING)) { > - disk = dmz_dev->bdev->bd_disk; > - if (blk_queue_dying(bdev_get_queue(dmz_dev->bdev))) { > - dmz_dev_warn(dmz_dev, "Backing device queue dying"); > - dmz_dev->flags |= DMZ_BDEV_DYING; > - } else if (disk->fops->check_events) { > - if (disk->fops->check_events(disk, 0) & > - DISK_EVENT_MEDIA_CHANGE) { > - dmz_dev_warn(dmz_dev, "Backing device offline"); > - dmz_dev->flags |= DMZ_BDEV_DYING; > - } > - } > + if (dmz_dev->flags & DMZ_CHECK_BDEV) > + return !dmz_check_bdev(dmz_dev); > + > + if (blk_queue_dying(bdev_get_queue(dmz_dev->bdev))) { > + dmz_dev_warn(dmz_dev, "Backing device queue dying"); > + dmz_dev->flags |= DMZ_BDEV_DYING; > } > > return dmz_dev->flags & DMZ_BDEV_DYING; > } > > +/* > + * Check the backing device availability. This detects such events as > + * backing device going offline due to errors, media removals, etc. > + * This check is less efficient than dmz_bdev_is_dying() and should > + * only be performed as a part of error handling. > + */ > +bool dmz_check_bdev(struct dmz_dev *dmz_dev) > +{ > + struct gendisk *disk; > + > + dmz_dev->flags &= ~DMZ_CHECK_BDEV; > + > + if (dmz_bdev_is_dying(dmz_dev)) > + return false; > + > + disk = dmz_dev->bdev->bd_disk; > + if (disk->fops->check_events && > + disk->fops->check_events(disk, 0) & DISK_EVENT_MEDIA_CHANGE) { > + dmz_dev_warn(dmz_dev, "Backing device offline"); > + dmz_dev->flags |= DMZ_BDEV_DYING; > + } > + > + return !(dmz_dev->flags & DMZ_BDEV_DYING); > +} > + > /* > * Process a new BIO. > */ > @@ -907,8 +929,8 @@ static int dmz_prepare_ioctl(struct dm_target *ti, struct > block_device **bdev) > { > struct dmz_target *dmz = ti->private; > > - if (dmz_bdev_is_dying(dmz->dev)) > - return -ENODEV; > + if (!dmz_check_bdev(dmz->dev)) > + return -EIO; > > *bdev = dmz->dev->bdev; > > diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h > index d8e70b0ade35..5b5e493d479c 100644 > --- a/drivers/md/dm-zoned.h > +++ b/drivers/md/dm-zoned.h > @@ -72,6 +72,7 @@ struct dmz_dev { > > /* Device flags. */ > #define DMZ_BDEV_DYING (1 << 0) > +#define DMZ_CHECK_BDEV (2 << 0) > > /* > * Zone descriptor. > @@ -255,5 +256,6 @@ void dmz_schedule_reclaim(struct dmz_reclaim *zrc); > * Functions defined in dm-zoned-target.c > */ > bool dmz_bdev_is_dying(struct dmz_dev *dmz_dev); > +bool dmz_check_bdev(struct dmz_dev *dmz_dev); > > #endif /* DM_ZONED_H */ > -- Damien Le Moal Western Digital Research -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
