Re: [RFC PATCH v3 8/9] md: Implement ->corrupted_range()

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Shiyang Ruan <ruansy.fnst@cn.fujitsu.com>,
	linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-nvdimm@lists.01.org, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, linux-raid@vger.kernel.org,
	dan.j.williams@intel.com, hch@lst.de, song@kernel.org,
	rgoldwyn@suse.de, qi.fuli@fujitsu.com, y-goto@fujitsu.com,
	Theodore Ts'o <tytso@mit.edu>
Subject: Re: [RFC PATCH v3 8/9] md: Implement ->corrupted_range()
Date: Wed, 16 Dec 2020 10:28:33 +1100	[thread overview]
Message-ID: <20201215232833.GM632069@dread.disaster.area> (raw)
In-Reply-To: <20201215205102.GB6918@magnolia>

On Tue, Dec 15, 2020 at 12:51:02PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 15, 2020 at 08:14:13PM +0800, Shiyang Ruan wrote:
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index 4688bff19c20..e8cfaf860149 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> > @@ -267,11 +267,14 @@ static int pmem_corrupted_range(struct gendisk *disk, struct block_device *bdev,
> >  
> >  	bdev_offset = (disk_sector - get_start_sect(bdev)) << SECTOR_SHIFT;
> >  	sb = get_super(bdev);
> > -	if (sb && sb->s_op->corrupted_range) {
> > +	if (!sb) {
> > +		rc = bd_disk_holder_corrupted_range(bdev, bdev_offset, len, data);
> > +		goto out;
> > +	} else if (sb->s_op->corrupted_range)
> >  		rc = sb->s_op->corrupted_range(sb, bdev, bdev_offset, len, data);
> > -		drop_super(sb);
> 
> This is out of scope for this patch(set) but do you think that the scsi
> disk driver should intercept media errors from sense data and call
> ->corrupted_range too?  ISTR Ted muttering that one of his employers had
> a patchset to do more with sense data than the upstream kernel currently
> does...

Most definitely!

That's the whole point of layering corrupt range reporting through
the device layers like this - the corrupted range reporting is not
limited specifically to pmem devices and so generic storage failures
(e.g.  RAID failures, hardware media failures, etc) can be reported
back up to the filesystem and we can take immediate, appropriate
action, including reporting to userspace that they just lost data in
file X at offset Y...

Combine that with the proposed "watch_sb()" syscall for reporting
such errors in a generic manner to interested listeners, and we've
got a fairly solid generic path for reporting data loss events to
userspace for an appropriate user-defined action to be taken...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com