Re: [PATCH RESEND v6 1/9] pagemap: Introduce ->memory_failure()

[h...@lst.de]

On Wed, Aug 18, 2021 at 10:10:51AM -0700, Dan Williams wrote:
> > Sounds like a nice solution.  I think I can add an is_notify_supported() 
> > interface in dax_holder_ops and check it when register dax_holder.
> 
> Shouldn't the fs avoid registering a memory failure handler if it is
> not prepared to take over? For example, shouldn't this case behave
> identically to ext4 that will not even register a callback?

Yes.

Re: [dm-devel] [PATCH 05/11] block: get rid of the trace rq insert wrapper

[h...@lst.de]

On Fri, Jul 03, 2020 at 11:29:25PM +, Chaitanya Kulkarni wrote:
> Christoph,
> 
> On 6/29/20 4:44 PM, Chaitanya Kulkarni wrote:
> > Get rid of the wrapper for trace_block_rq_insert() and call the function
> > directly.
> > 
> > Signed-off-by: Chaitanya Kulkarni
> > ---
> 
> Can we re-consider adding this patch ? here are couple of reasons:-
> 
> 1. Increase in the size of the text region of the object file:-
> 
> By adding the trace header #include 
> in io-scheduler where it is calling trace_block_rq_insert()
> increases the size of the text region of the object file
> kyber(+215) & bfq (+317) [1].

You really shouldn't have both loaded, never mind used at the same
time.  It also avoid a function call for the scheduler fast path.

> 2. Mandatory io-sched built-in kernel compilation:-
> 
> When testing with a different io-sched KConfig options ("*" vs "M"),
> when kyber and bfq compilation option set to "M" having this patch
> reports error[2].

See EXPORT_TRACEPOINT_SYMBOL_GPL.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 12/15] block: merge blk_types.h into bio.h

[h...@lst.de]

On Thu, May 18, 2017 at 02:25:52PM +, Bart Van Assche wrote:
> On Thu, 2017-05-18 at 15:18 +0200, Christoph Hellwig wrote:
> > We've cleaned up our headers sufficiently that we don't need this split
> > anymore.
> 
> Hello Christoph,
> 
> Request-based drivers need the structure definitions from  and
> the type definitions from  but do not need the definition 
> of
> struct bio. Have you considered to remove #include  from file
> include/linux/blkdev.h? Do you think that would help to reduce the kernel 
> build
> time?

Most block drivers end up needing bio.h anyway.  But I can skip this
part of the series for now.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 25/27] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Wed, May 10, 2017 at 09:50:35PM -0700, Nicholas A. Bellinger wrote:
> 1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
> support up to IBLOCK, in order to maintain SCSI target feature
> compatibility.

No way.  If you want to zero use REQ_OP_WRITE_ZEROES..

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 25/27] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Mon, May 08, 2017 at 11:46:14PM -0700, Nicholas A. Bellinger wrote:
> That said, simply propagating up q->limits.max_write_zeroes_sectors as
> dev_attrib->unmap_zeroes_data following existing code still looks like
> the right thing to do.

It is not.  Martin has decoupled write same/zeroes support from discard
support.  Any device will claim to support it initially, and we'll
only clear the flag if a Write Same command fails.

So even if LBPRZ is not set you can trivially get into a situation
where discard is supported through UNMAP, and you'll incorrectly
set LBPRZ and will cause data corruption.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 25/27] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Tue, May 02, 2017 at 08:33:15PM -0700, Nicholas A. Bellinger wrote:
> The larger target/iblock conversion patch looks like post v4.12 material
> at this point, so to avoid breakage wrt to existing LBPRZ behavior, I'll
> plan to push the following patch post -rc1.

I don't think this is safe.  If you want to do the aboe you also
need to ensure ->execute_unmap always zeroes the data.  For actual
files in the file backend we should be all fine, but for the block
device case [1] and iblock we'd need to use blkdev_issue_zeroout
instead of blkdev_issue_discard when unmap_zeroes_data is set.

[1] which btw already seems broken as it doesn't invalidate cached
data when issuing a discard.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 25/27] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Tue, May 02, 2017 at 12:16:13AM -0700, Nicholas A. Bellinger wrote:
> Or, another options is use bdev_write_zeroes_sectors() to determine when
> dev_attrib->unmap_zeroes_data should be set.

Yes, that in combination with your patch to use bdev_write_zeroes_sectors
for zeroing from write same seems like the right fix.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] small dm mpath cleanups

[h...@lst.de]

On Wed, Apr 26, 2017 at 06:41:27PM +, Bart Van Assche wrote:
> On Wed, 2017-04-26 at 09:40 +0200, Christoph Hellwig wrote:
> > this series has some prep patches for my work to have proper, type
> > checked block errors codes.  One fallout of that is that we need to
> > get rid of how dm overloads a few return values with either internal
> > positive error codes or negative errno values.  This patches does
> > that, which happens to clean things up a bit, and also allows us
> > dm to propagate the actual error code in one case where it currently
> > is dropped on the floor.
> 
> Hello Christoph,
> 
> Some patches in this series conflict with patches I would like to end up in
> the stable kernel series. If I would rebase my patch series on top of your
> series then that would make it harder to apply my patches on the stable
> kernel trees. Mike and Christoph, please advise how to proceed.

Bugfixes always go before cleanups.  I'd be happy to delay and/or rebase
any of my patches as needed.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 02/23] block: remove the blk_execute_rq return value

[h...@lst.de]

On Wed, Apr 19, 2017 at 09:07:45PM +, Bart Van Assche wrote:
> > +   blk_execute_rq(or->request->q, NULL, or->request, 0);
> > +   error = or->request ? -EIO : 0;
> 
> Hello Christoph,
> 
> Did you perhaps intend or->request->errors instead of or->request?

Yes, thanks.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] blk-mq: remove the error argument to blk_mq_complete_request

[h...@lst.de]

On Tue, Apr 18, 2017 at 10:50:18PM +, Bart Van Assche wrote:
> On Tue, 2017-04-18 at 08:52 -0700, Christoph Hellwig wrote:
> > Now that we always have a ->complete callback we can remove the direct
> > call to blk_mq_end_request, as well as the error argument to
> > blk_mq_complete_request.
> 
> Hello Christoph,
> 
> Please add a runtime check that issues a warning early if a .complete
> callback function is missing.
> 
> Are you sure that all blk-mq drivers have a .complete callback? In the
> request-errors branch of your block git tree I found the following:

I think the problem is that my above changelog was wrong - we only
require the complete_rq method if the driver calls the
blk_mq_complete_request function.  Both rbd and ubiblock don't in
the tree.  They probably should, but that's work for a separate
series.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] block: add a error_count field to struct request

[h...@lst.de]

On Tue, Apr 18, 2017 at 10:57:11PM +, Bart Van Assche wrote:
> Both blk-mq and the traditional block layer support a .cmd_size field to
> make the block layer core allocate driver-specific data at the end of struct
> request. Could that mechanism have been used for the error_count field?

It could, and that's what I did for the modern drivers.  It would have
been a bit of a pain for these old floppy drivers, though.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] scsi: introduce a new result field in struct scsi_request

[h...@lst.de]

On Tue, Apr 18, 2017 at 10:34:20PM +, Bart Van Assche wrote:
> Did you perhaps intend "req->result" instead of "rq->result"?

Yes.

> Did you intend "war" or is that perhaps a typo?

I'll fix the comment.

> > trace_scsi_dispatch_cmd_done(cmd);
> > -   blk_mq_complete_request(cmd->request, cmd->request->errors);
> > +   blk_mq_complete_request(cmd->request, 0);
> >  }
> 
> Why has cmd->request->errors been changed into 0?

Because the argument is only used to set req->errors, which we won't
look at any more.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] block: remove the blk_execute_rq return value

[h...@lst.de]

On Tue, Apr 18, 2017 at 10:24:15PM +, Bart Van Assche wrote:
> On Tue, 2017-04-18 at 08:52 -0700, Christoph Hellwig wrote:
> > diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
> > index b05e151c9b38..82c6d02193ae 100644
> > --- a/drivers/block/paride/pd.c
> > +++ b/drivers/block/paride/pd.c
> > @@ -747,7 +747,8 @@ static int pd_special_command(struct pd_unit *disk,
> >  
> > rq->special = func;
> >  
> > -   err = blk_execute_rq(disk->gd->queue, disk->gd, rq, 0);
> > +   blk_execute_rq(disk->gd->queue, disk->gd, rq, 0);
> > +   err = req->errors ? -EIO : 0;
> >  
> > blk_put_request(rq);
> > return err;
> 
> Hello Christoph,
> 
> Sorry that I hadn't noticed this before but shouldn't "req" be changed into
> "rq"? Otherwise this patch looks fine to me. If this comment gets addressed
> you can add:

It should.  But it seems this no one caught this because the check gets
removed later in the series by "pd: remove bogus check for req->errors",
so I should just move that patch earlier in the series.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 02/25] block: remove the blk_execute_rq return value

[h...@lst.de]

On Mon, Apr 17, 2017 at 10:01:09AM -0600, Jens Axboe wrote:
> Are you respinning this series for 4.12?

Yes, I think I got enough feedback by now to resend it.  I'll try to
get it out today.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 01/25] remove the mg_disk driver

[h...@lst.de]

On Thu, Apr 13, 2017 at 07:58:13PM +, Bart Van Assche wrote:
> Should the person who submitted this driver be CC-ed for this patch (unsik
> Kim )?

Yes, he should.  And in fact he was when I sent this patch out separately
a little earlier, I just included it in this series for reference.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 12/23] sd: handle REQ_UNMAP

[h...@lst.de]

On Thu, Mar 30, 2017 at 11:28:32AM -0400, Martin K. Petersen wrote:
> "h...@lst.de" <h...@lst.de> writes:
> 
> Christoph,
> 
> > On Tue, Mar 28, 2017 at 04:48:55PM +, Bart Van Assche wrote:
> >> >  if (sdp->no_write_same)
> >> >  return BLKPREP_INVALID;
> >> >  if (sdkp->ws16 || sector > 0x || nr_sectors > 0x)
> >> 
> >> Users can change the provisioning mode from user space from SD_LBP_WS16 
> >> into
> >> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
> >> 0x || nr_sectors > 0x) check if REQ_UNMAP is set.
> >
> > They can, and if the device has too many sectors that will already cause
> > discard to fail,
> 
> I'm not sure I understand what you mean by that?

If you manually change the provisioning mode to WS10 on a device that
must use WRITE SAME (16) to be able to address all blocks you're already
screwed right now, and with this patch you can screw yourself through
the WRITE_ZEROES path in addition to the DISCARD path.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 23/23] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Thu, Mar 30, 2017 at 11:29:29AM -0400, Martin K. Petersen wrote:
> "h...@lst.de" <h...@lst.de> writes:
> 
> > Jens, any opinion?  I'd like to remove it too, but I fear it might
> > break things.  We could deprecate it first with a warning when read
> > and then remove it a few releases down the road.
> 
> I know of several apps that check this variable (as opposed to the
> ioctl).

The above was in reference to both methods..

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 23/23] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Tue, Mar 28, 2017 at 05:00:48PM +, Bart Van Assche wrote:
> 
> It seems to me like the documentation in Documentation/ABI/testing/sysfs-block
> and the above code are not in sync. I think the above code will cause reading
> from the discard_zeroes_data attribute to return an empty string ("") instead
> of "0\n".

Thanks, fine with me.

> 
> BTW, my personal preference is to remove this attribute entirely because 
> keeping
> it will cause confusion, no matter how well we document the behavior of this
> attribute.

Jens, any opinion?  I'd like to remove it too, but I fear it might
break things.  We could deprecate it first with a warning when read
and then remove it a few releases down the road.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 12/23] sd: handle REQ_UNMAP

[h...@lst.de]

On Tue, Mar 28, 2017 at 04:48:55PM +, Bart Van Assche wrote:
> > if (sdp->no_write_same)
> > return BLKPREP_INVALID;
> > if (sdkp->ws16 || sector > 0x || nr_sectors > 0x)
> 
> Users can change the provisioning mode from user space from SD_LBP_WS16 into
> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
> 0x || nr_sectors > 0x) check if REQ_UNMAP is set.

They can, and if the device has too many sectors that will already cause
discard to fail, and in this case it will cause write zeroes to fail as
well.  The intent behind this patch is to keep the behavior the same
as the old path that uses discards for zeroing.  The logic looks a bit
clumsy, but I'd rather keep it as-is.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 11/23] block_dev: use blkdev_issue_zerout for hole punches

[h...@lst.de]

On Tue, Mar 28, 2017 at 04:50:47PM +, Bart Van Assche wrote:
> On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> > This gets us support for non-discard efficient write of zeroes (e.g. NVMe)
> > and preparse for removing the discard_zeroes_data flag.
> 
> Hello Christoph,
> 
> "preparse" probably should have been "prepare"?

Yes, fixed.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] split scsi passthrough fields out of struct request V2

[h...@lst.de]

On Fri, Jan 27, 2017 at 09:27:53PM +, Bart Van Assche wrote:
> Have you considered to convert all block drivers to the new
> approach and to get rid of request.special? If so, do you already
> have plans to start working on this? I'm namely wondering wheter I
> should start working on this myself.

Hi Bart,

I'd love to have all drivers move of using .special (and thus reducing
request size further).  I think the general way to do that is to convert
them to blk-mq and not using the legacy cmd_size field.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] split scsi passthrough fields out of struct request V2

[h...@lst.de]

On Thu, Jan 26, 2017 at 11:57:36AM -0700, Jens Axboe wrote:
> It's against my for-4.11/block, which you were running under Christoph's
> patches. Maybe he's using an older version? In any case, should be
> pretty trivial for you to hand apply. Just ensure that .flags is set to
> 0 for the common cases, and inherit 'flags' when it is passed in.

No, the flush op cleanups you asked for last round create a conflict
with your patch.  They should be trivial to fix, though.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel