linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Ruan Shiyang <ruansy.fnst@cn.fujitsu.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: <linux-kernel@vger.kernel.org>, <linux-xfs@vger.kernel.org>,
	<linux-nvdimm@lists.01.org>, <linux-mm@kvack.org>,
	<linux-fsdevel@vger.kernel.org>, <dm-devel@redhat.com>,
	<darrick.wong@oracle.com>, <dan.j.williams@intel.com>,
	<david@fromorbit.com>, <hch@lst.de>, <agk@redhat.com>,
	<snitzer@redhat.com>, <rgoldwyn@suse.de>, <qi.fuli@fujitsu.com>,
	<y-goto@fujitsu.com>
Subject: Re: [PATCH RESEND v2 08/10] md: Implement ->corrupted_range()
Date: Wed, 3 Feb 2021 09:51:37 +0800	[thread overview]
Message-ID: <8742625e-8ae7-47a8-fd62-18c201c45a33@cn.fujitsu.com> (raw)
In-Reply-To: <20210202031711.GJ7193@magnolia>



On 2021/2/2 上午11:17, Darrick J. Wong wrote:
> On Fri, Jan 29, 2021 at 02:27:55PM +0800, Shiyang Ruan wrote:
>> With the support of ->rmap(), it is possible to obtain the superblock on
>> a mapped device.
>>
>> If a pmem device is used as one target of mapped device, we cannot
>> obtain its superblock directly.  With the help of SYSFS, the mapped
>> device can be found on the target devices.  So, we iterate the
>> bdev->bd_holder_disks to obtain its mapped device.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@cn.fujitsu.com>
>> ---
>>   drivers/md/dm.c       | 61 +++++++++++++++++++++++++++++++++++++++++++
>>   drivers/nvdimm/pmem.c | 11 +++-----
>>   fs/block_dev.c        | 42 ++++++++++++++++++++++++++++-
> 
> I feel like this ^^^ part that implements the generic ability for a block
> device with a bad sector to notify whatever's holding onto it (fs, other
> block device) should be in patch 2.  That's generic block layer code,
> and it's hard to tell (when you're looking at patch 2) what the bare
> function declaration in it is really supposed to do.
> 
> Also, this patch is still difficult to review because it mixes device
> mapper, nvdimm, and block layer changes!

OK.  I'll split this to make it looks simple.

> 
>>   include/linux/genhd.h |  2 ++
>>   4 files changed, 107 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 7bac564f3faa..31b0c340b695 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -507,6 +507,66 @@ static int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
>>   #define dm_blk_report_zones		NULL
>>   #endif /* CONFIG_BLK_DEV_ZONED */
>>   
>> +struct corrupted_hit_info {
>> +	struct block_device *bdev;
>> +	sector_t offset;
>> +};
>> +
>> +static int dm_blk_corrupted_hit(struct dm_target *ti, struct dm_dev *dev,
>> +				sector_t start, sector_t count, void *data)
>> +{
>> +	struct corrupted_hit_info *bc = data;
>> +
>> +	return bc->bdev == (void *)dev->bdev &&
>> +			(start <= bc->offset && bc->offset < start + count);
>> +
>> +}
>> +
>> +struct corrupted_do_info {
>> +	size_t length;
>> +	void *data;
>> +};
>> +
>> +static int dm_blk_corrupted_do(struct dm_target *ti, struct block_device *bdev,
>> +			       sector_t disk_sect, void *data)
>> +{
>> +	struct corrupted_do_info *bc = data;
>> +	loff_t disk_off = to_bytes(disk_sect);
>> +	loff_t bdev_off = to_bytes(disk_sect - get_start_sect(bdev));
>> +
>> +	return bd_corrupted_range(bdev, disk_off, bdev_off, bc->length, bc->data);
>> +}
>> +
>> +static int dm_blk_corrupted_range(struct gendisk *disk,
>> +				  struct block_device *target_bdev,
>> +				  loff_t target_offset, size_t len, void *data)
>> +{
>> +	struct mapped_device *md = disk->private_data;
>> +	struct dm_table *map;
>> +	struct dm_target *ti;
>> +	sector_t target_sect = to_sector(target_offset);
>> +	struct corrupted_hit_info hi = {target_bdev, target_sect};
>> +	struct corrupted_do_info di = {len, data};
>> +	int srcu_idx, i, rc = -ENODEV;
>> +
>> +	map = dm_get_live_table(md, &srcu_idx);
>> +	if (!map)
>> +		return rc;
>> +
>> +	for (i = 0; i < dm_table_get_num_targets(map); i++) {
>> +		ti = dm_table_get_target(map, i);
>> +		if (!(ti->type->iterate_devices && ti->type->rmap))
>> +			continue;
>> +		if (!ti->type->iterate_devices(ti, dm_blk_corrupted_hit, &hi))
>> +			continue;
>> +
>> +		rc = ti->type->rmap(ti, target_sect, dm_blk_corrupted_do, &di);
> 
> Why is it necessary to call ->iterate_devices here?

->iterate_devices() here is to find out which target is the pmem device 
which is corrupted now.  Then call ->rmap() on this target.  Other 
targets will be ignored.

> 
> If you pass the target_bdev, offset, and length to the dm-target's
> ->rmap function, it should be able to work backwards through its mapping
> logic to come up with all the LBA ranges of the mapped_device that
> are affected, and then it can call bd_corrupted_range on each of those
> reverse mappings.
> 
> It would be helpful to have the changes to dm-linear.c in this patch
> too, since that's the only real implementation at this point.
> 
>> +		break;
>> +	}
>> +
>> +	dm_put_live_table(md, srcu_idx);
>> +	return rc;
>> +}
>> +
>>   static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
>>   			    struct block_device **bdev)
>>   {
>> @@ -3062,6 +3122,7 @@ static const struct block_device_operations dm_blk_dops = {
>>   	.getgeo = dm_blk_getgeo,
>>   	.report_zones = dm_blk_report_zones,
>>   	.pr_ops = &dm_pr_ops,
>> +	.corrupted_range = dm_blk_corrupted_range,
>>   	.owner = THIS_MODULE
>>   };
>>   
>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
>> index 501959947d48..3d9f4ccbbd9e 100644
>> --- a/drivers/nvdimm/pmem.c
>> +++ b/drivers/nvdimm/pmem.c
>> @@ -256,21 +256,16 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
>>   static int pmem_corrupted_range(struct gendisk *disk, struct block_device *bdev,
>>   				loff_t disk_offset, size_t len, void *data)
>>   {
>> -	struct super_block *sb;
>>   	loff_t bdev_offset;
>>   	sector_t disk_sector = disk_offset >> SECTOR_SHIFT;
>> -	int rc = 0;
>> +	int rc = -ENODEV;
>>   
>>   	bdev = bdget_disk_sector(disk, disk_sector);
>>   	if (!bdev)
>> -		return -ENODEV;
>> +		return rc;
>>   
>>   	bdev_offset = (disk_sector - get_start_sect(bdev)) << SECTOR_SHIFT;
>> -	sb = get_super(bdev);
>> -	if (sb && sb->s_op->corrupted_range) {
>> -		rc = sb->s_op->corrupted_range(sb, bdev, bdev_offset, len, data);
>> -		drop_super(sb);
>> -	}
>> +	rc = bd_corrupted_range(bdev, bdev_offset, bdev_offset, len, data);
>>   
>>   	bdput(bdev);
>>   	return rc;
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 3b8963e228a1..3cc2b2911e3a 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -1079,6 +1079,27 @@ struct bd_holder_disk {
>>   	int			refcnt;
>>   };
>>   
>> +static int bd_disk_holder_corrupted_range(struct block_device *bdev, loff_t off,
>> +					  size_t len, void *data)
>> +{
>> +	struct bd_holder_disk *holder;
>> +	struct gendisk *disk;
>> +	int rc = 0;
>> +
>> +	if (list_empty(&(bdev->bd_holder_disks)))
>> +		return -ENODEV;
>> +
>> +	list_for_each_entry(holder, &bdev->bd_holder_disks, list) {
>> +		disk = holder->disk;
>> +		if (disk->fops->corrupted_range) {
>> +			rc = disk->fops->corrupted_range(disk, bdev, off, len, data);
>> +			if (rc != -ENODEV)
>> +				break;
>> +		}
>> +	}
>> +	return rc;
>> +}
>> +
>>   static struct bd_holder_disk *bd_find_holder_disk(struct block_device *bdev,
>>   						  struct gendisk *disk)
>>   {
>> @@ -1212,7 +1233,26 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk)
>>   	mutex_unlock(&bdev->bd_mutex);
>>   }
>>   EXPORT_SYMBOL_GPL(bd_unlink_disk_holder);
>> -#endif
>> +#endif /* CONFIG_SYSFS */
>> +
>> +int bd_corrupted_range(struct block_device *bdev, loff_t disk_off,
>> +		       loff_t bdev_off, size_t len, void *data)
>> +{
>> +	struct super_block *sb = get_super(bdev);
>> +	int rc = -EOPNOTSUPP;
>> +
>> +	if (!sb) {
>> +#ifdef CONFIG_SYSFS
>> +		rc = bd_disk_holder_corrupted_range(bdev, disk_off, len, data);
>> +#endif /* CONFIG_SYSFS */
> 
> Normal kernel convention is that you'd provide a empty shell for the
> CONFIG_SYSFS=n case, e.g.
> 
> #ifdef CONFIG_SYSFS
> int bd_corrupted_range(...) {
> 	/* real code */
> }
> #else
> static inline bd_corrupted_range(...) { return -EOPNOTSUPP; }
> #endif
> 
> so that you don't have preprocessor directives making this function
> choppy.

I'll fix it.


--
Thanks,
Ruan Shiyang.

> 
> --D
> 
>> +		return rc;
>> +	} else if (sb->s_op->corrupted_range)
>> +		rc = sb->s_op->corrupted_range(sb, bdev, bdev_off, len, data);
>> +	drop_super(sb);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL(bd_corrupted_range);
>>   
>>   static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>>   
>> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
>> index 4da480798955..996f91b08d48 100644
>> --- a/include/linux/genhd.h
>> +++ b/include/linux/genhd.h
>> @@ -315,6 +315,8 @@ void unregister_blkdev(unsigned int major, const char *name);
>>   bool bdev_check_media_change(struct block_device *bdev);
>>   int __invalidate_device(struct block_device *bdev, bool kill_dirty);
>>   void set_capacity(struct gendisk *disk, sector_t size);
>> +int bd_corrupted_range(struct block_device *bdev, loff_t disk_off,
>> +		       loff_t bdev_off, size_t len, void *data);
>>   
>>   /* for drivers/char/raw.c: */
>>   int blkdev_ioctl(struct block_device *, fmode_t, unsigned, unsigned long);
>> -- 
>> 2.30.0
>>
>>
>>
> 
> 




  reply	other threads:[~2021-02-03  1:51 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-29  6:27 [PATCH RESEND v2 00/10] fsdax: introduce fs query to support reflink Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 01/10] pagemap: Introduce ->memory_failure() Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 02/10] blk: Introduce ->corrupted_range() for block device Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 03/10] fs: Introduce ->corrupted_range() for superblock Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 04/10] mm, fsdax: Refactor memory-failure handler for dax mapping Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 05/10] mm, pmem: Implement ->memory_failure() in pmem driver Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 06/10] pmem: Implement ->corrupted_range() for " Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 07/10] dm: Introduce ->rmap() to find bdev offset Shiyang Ruan
2021-01-29  6:27 ` [PATCH RESEND v2 08/10] md: Implement ->corrupted_range() Shiyang Ruan
2021-02-02  3:17   ` Darrick J. Wong
2021-02-03  1:51     ` Ruan Shiyang [this message]
2021-01-29  6:27 ` [PATCH RESEND v2 09/10] xfs: Implement ->corrupted_range() for XFS Shiyang Ruan
2021-02-02  2:41   ` Darrick J. Wong
2021-02-02 12:48     ` Ruan Shiyang
2021-01-29  6:27 ` [PATCH RESEND v2 10/10] fs/dax: Remove useless functions Shiyang Ruan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8742625e-8ae7-47a8-fd62-18c201c45a33@cn.fujitsu.com \
    --to=ruansy.fnst@cn.fujitsu.com \
    --cc=agk@redhat.com \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=djwong@kernel.org \
    --cc=dm-devel@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=qi.fuli@fujitsu.com \
    --cc=rgoldwyn@suse.de \
    --cc=snitzer@redhat.com \
    --cc=y-goto@fujitsu.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).