Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Keith Busch 
On Mon, Jan 08, 2018 at 04:34:36PM +0100, Christoph Hellwig wrote:
> It's basically a kernel bug as it tries to access lbas that do not
> exist.  BLK_STS_TARGET should be fine.

Okay, I'll fix this and address your other comments, and resend. Thanks
for the feedback.

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

Re: [dm-devel] [PATCH v6 2/2] dm: allow device-mapper to operate without dax support

2018-01-08 Thread Mike Snitzer 
On Wed, Nov 29 2017 at  2:00pm -0500,
Dan Williams  wrote:

> Change device-mapper's DAX dependency to require the presence of at
> least one DAX_DRIVER. This allows device-mapper to be built without
> bringing the DAX core along which is especially wasteful when there are
> no DAX drivers, like BLK_DEV_PMEM, configured.
> 
> Cc: Alasdair Kergon 
> Cc: Mike Snitzer 
> Reported-by: Bart Van Assche 
> Reported-by: kbuild test robot 
> Signed-off-by: Dan Williams 

Reviewed-by: Mike Snitzer 

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

Re: [dm-devel] [PATCH v6 1/2] dax: introduce CONFIG_DAX_DRIVER

2018-01-08 Thread Mike Snitzer 
On Wed, Nov 29 2017 at  2:00pm -0500,
Dan Williams  wrote:

> In support of allowing device-mapper to compile out idle/dead code when
> there are no dax providers in the system, introduce the DAX_DRIVER
> symbol. This is selected by all leaf drivers that device-mapper might be
> layered on top. This allows device-mapper to conditionally 'select DAX'
> only when a provider is present.
> 
> Cc: Paul Mackerras 
> Cc: Michael Ellerman 
> Cc: Martin Schwidefsky 
> Cc: Heiko Carstens 
> Cc: Gerald Schaefer 
> Cc: Benjamin Herrenschmidt 
> Cc: Mike Snitzer 
> Cc: Bart Van Assche 
> Signed-off-by: Dan Williams 

Reviewed-by: Mike Snitzer 

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

Re: [dm-devel] [PATCH v6 0/2] dax, dm: stop requiring dax for device-mapper

2018-01-08 Thread Mike Snitzer 
On Sun, Jan 07 2018 at  3:31pm -0500,
Dan Williams  wrote:

> On Thu, Jan 4, 2018 at 10:12 AM, Mike Snitzer  wrote:
> > On Wed, Nov 29 2017 at  1:59pm -0500,
> > Dan Williams  wrote:
> >
> >> Changes since v5 [1]:
> >> * Make DAX_DRIVER select DAX to simplify the Kconfig dependencies
> >>   (Michael)
> >> * Rebase on 4.15-rc1 and add new IS_ENABLED(CONFIG_DAX_DRIVER) checks in
> >>   drivers/md/dm-log-writes.c.
> >>
> >> [1]: https://lists.01.org/pipermail/linux-nvdimm/2017-September/012569.html
> >>
> >> ---
> >>
> >> Hi Mike,
> >>
> >> Bart points out that the DAX core is unconditionally enabled if
> >> device-mapper is enabled. Add some config machinery and some
> >> stub/static-inline routines to allow dax infrastructure to be deleted
> >> from device-mapper at compile time.
> >
> > Hey Dan,
> >
> > Did you want me to pick this up for 4.16 or were you thinking of sending
> > the changes through a different tree?
> >
> > I'm fine with picking these up so long as you don't expect the dax.h and
> > Kconfig changes to become a source of conflict come merge time.
> 
> There might be some more collisions as we are going to kill the DAX
> support in axonram and dcssblk. If I get your reviewed-by on these
> I'll take them through the nvdimm tree.

OK, I've dropped these and will respond with my Reviewed-by:s.

Thanks.

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Christoph Hellwig 
On Mon, Jan 08, 2018 at 10:29:33AM -0500, Mike Snitzer wrote:
> No argument needed.  Definitely needs fixing.  Too many upper layers
> consider BLK_STS_NOSPC retryable (XFS, ext4, dm-thinp, etc).  Which
> NVME_SC_LBA_RANGE absolutely isn't.
> 
> When I backfilled NVME_SC_LBA_RANGE handling I categorized it as
> BLK_STS_TARGET.  Do you have a better suggestion for how
> NVME_SC_LBA_RANGE should be categorized?

It's basically a kernel bug as it tries to access lbas that do not
exist.  BLK_STS_TARGET should be fine.

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Mike Snitzer 
On Mon, Jan 08 2018 at  5:19am -0500,
Christoph Hellwig  wrote:

> On Mon, Jan 08, 2018 at 11:09:03AM +0100, Hannes Reinecke wrote:
> > >>  case NVME_SC_SUCCESS:
> > >>  return BLK_STS_OK;
> > >>  case NVME_SC_CAP_EXCEEDED:
> > >> +case NVME_SC_LBA_RANGE:
> > >>  return BLK_STS_NOSPC;
> > > 
> > > lba range isn't really enospc.  It is returned when the lba in
> > > the command is outside the logical size of the namespace.
> > > 
> > Isn't that distinction pretty academic?
> > The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...
> 
> Yes, BLK_STS_NOSPC matters.  And the fix is pretty trivial, so there is
> no point in arguing.

No argument needed.  Definitely needs fixing.  Too many upper layers
consider BLK_STS_NOSPC retryable (XFS, ext4, dm-thinp, etc).  Which
NVME_SC_LBA_RANGE absolutely isn't.

When I backfilled NVME_SC_LBA_RANGE handling I categorized it as
BLK_STS_TARGET.  Do you have a better suggestion for how
NVME_SC_LBA_RANGE should be categorized?

Mike

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Hannes Reinecke 
On 01/08/2018 10:55 AM, Christoph Hellwig wrote:
> On Thu, Jan 04, 2018 at 03:46:19PM -0700, Keith Busch wrote:
>> This adds more NVMe status code translations to blk_status_t values,
>> and captures all the current status codes NVMe multipath uses.
>>
>> Signed-off-by: Keith Busch 
>> ---
>>  drivers/nvme/host/core.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 2a69d735efbc..f1cf1f6c5b28 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request 
>> *req)
>>  case NVME_SC_SUCCESS:
>>  return BLK_STS_OK;
>>  case NVME_SC_CAP_EXCEEDED:
>> +case NVME_SC_LBA_RANGE:
>>  return BLK_STS_NOSPC;
> 
> lba range isn't really enospc.  It is returned when the lba in
> the command is outside the logical size of the namespace.
> 
Isn't that distinction pretty academic?
The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Christoph Hellwig 
On Mon, Jan 08, 2018 at 11:09:03AM +0100, Hannes Reinecke wrote:
> >>case NVME_SC_SUCCESS:
> >>return BLK_STS_OK;
> >>case NVME_SC_CAP_EXCEEDED:
> >> +  case NVME_SC_LBA_RANGE:
> >>return BLK_STS_NOSPC;
> > 
> > lba range isn't really enospc.  It is returned when the lba in
> > the command is outside the logical size of the namespace.
> > 
> Isn't that distinction pretty academic?
> The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...

Yes, BLK_STS_NOSPC matters.  And the fix is pretty trivial, so there is
no point in arguing.

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Christoph Hellwig 
On Thu, Jan 04, 2018 at 03:46:19PM -0700, Keith Busch wrote:
> This adds more NVMe status code translations to blk_status_t values,
> and captures all the current status codes NVMe multipath uses.
> 
> Signed-off-by: Keith Busch 
> ---
>  drivers/nvme/host/core.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2a69d735efbc..f1cf1f6c5b28 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request 
> *req)
>   case NVME_SC_SUCCESS:
>   return BLK_STS_OK;
>   case NVME_SC_CAP_EXCEEDED:
> + case NVME_SC_LBA_RANGE:
>   return BLK_STS_NOSPC;

lba range isn't really enospc.  It is returned when the lba in
the command is outside the logical size of the namespace.

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

Re: [dm-devel] [PATCH 0/5] Failover criteria unification

2018-01-08 Thread Christoph Hellwig 
On Thu, Jan 04, 2018 at 04:47:27PM -0700, Keith Busch wrote:
> It looks like you can also touch up dm to allow it to multipath nvme
> even if CONFIG_NVME_MULTIPATH is set. It may be useful since native NVMe
> doesn't multipath namespaces across subsystems, and some crack smoking
> people want to create something that requires that.

Right now that is simply not allowed by the spec.  If Dave comes up
with a palatable way to retrofit that into the spec we'll support it.

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

Re: [dm-devel] [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

2018-01-08 Thread Christoph Hellwig 
> - if (unlikely(nvme_req(req)->status && nvme_req_needs_retry(req))) {
> - if (nvme_req_needs_failover(req)) {
> + blk_status_t status = nvme_error_status(req);
> +
> + if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
> + if (nvme_req_needs_failover(req, status)) {

We don't really need to call nvme_error_status if nvme_req(req)->status
is zero.

> -static inline bool nvme_req_needs_failover(struct request *req)
> +static inline bool nvme_req_needs_failover(struct request *req, blk_status_t 
> error)

line break after 80 characters, please.

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

Re: [dm-devel] [PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

2018-01-08 Thread Christoph Hellwig 
> +static inline bool blk_retryable(blk_status_t error)

The naming isn't really useful - it is about the fact that it's
worth retrying on another path.  So please chose a better name,
and add a kerneldoc comment describing it.

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

Re: [dm-devel] [PATCH 5/5] dm mpath: Use blk_retryable

2018-01-08 Thread Hannes Reinecke 
On 01/04/2018 11:46 PM, Keith Busch wrote:
> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch 
> ---
>  drivers/md/dm-mpath.c | 19 ++-
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 4/5] nvme/multipath: Use blk_retryable

2018-01-08 Thread Hannes Reinecke 
On 01/04/2018 11:46 PM, Keith Busch wrote:
> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch 
> ---
>  drivers/nvme/host/multipath.c | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index ae9abb600c0f..93bb72b6efb6 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -37,19 +37,7 @@ bool nvme_req_needs_failover(struct request *req, 
> blk_status_t error)
>  {
>   if (!(req->cmd_flags & REQ_NVME_MPATH))
>   return false;
> -
> - switch (error) {
> - case BLK_STS_NOTSUPP:
> - case BLK_STS_NOSPC:
> - case BLK_STS_TARGET:
> - case BLK_STS_NEXUS:
> - case BLK_STS_MEDIUM:
> - case BLK_STS_PROTECTION:
> - return false;
> - }
> -
> - /* Everything else could be a path failure, so should be retried */
> - return true;
> + return blk_retryable(error);
>  }
>  
>  void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

2018-01-08 Thread Hannes Reinecke 
On 01/04/2018 11:46 PM, Keith Busch wrote:
> This removes nvme multipath's specific status decoding to see if failover
> is needed, using the generic blk_status_t that was translated earlier. This
> abstraction from the raw NVMe status means nvme status decoding exists
> in just one place.
> 
> Signed-off-by: Keith Busch 
> ---
>  drivers/nvme/host/core.c  |  9 +
>  drivers/nvme/host/multipath.c | 44 
> ---
>  drivers/nvme/host/nvme.h  |  4 ++--
>  3 files changed, 15 insertions(+), 42 deletions(-)
> Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 1/5] nvme: Add more command status translation

2018-01-08 Thread Hannes Reinecke 
On 01/04/2018 11:46 PM, Keith Busch wrote:
> This adds more NVMe status code translations to blk_status_t values,
> and captures all the current status codes NVMe multipath uses.
> 
> Signed-off-by: Keith Busch 
> ---
>  drivers/nvme/host/core.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2a69d735efbc..f1cf1f6c5b28 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request 
> *req)
>   case NVME_SC_SUCCESS:
>   return BLK_STS_OK;
>   case NVME_SC_CAP_EXCEEDED:
> + case NVME_SC_LBA_RANGE:
>   return BLK_STS_NOSPC;
> + case NVME_SC_BAD_ATTRIBUTES:
>   case NVME_SC_ONCS_NOT_SUPPORTED:
> + case NVME_SC_INVALID_OPCODE:
> + case NVME_SC_INVALID_FIELD:
> + case NVME_SC_INVALID_NS:
>   return BLK_STS_NOTSUPP;
>   case NVME_SC_WRITE_FAULT:
>   case NVME_SC_READ_ERROR:
>   case NVME_SC_UNWRITTEN_BLOCK:
>   case NVME_SC_ACCESS_DENIED:
>   case NVME_SC_READ_ONLY:
> + case NVME_SC_COMPARE_FAILED:
>   return BLK_STS_MEDIUM;
>   case NVME_SC_GUARD_CHECK:
>   case NVME_SC_APPTAG_CHECK:
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

2018-01-08 Thread Hannes Reinecke 
On 01/04/2018 11:46 PM, Keith Busch wrote:
> This patch provides a common decoder for block status that may be retried
> so various entities wishing to consult this do not have to duplicate
> this decision.
> 
> Signed-off-by: Keith Busch 
> ---
>  include/linux/blk_types.h | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index a1e628e032da..b6a8723b493c 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -39,6 +39,22 @@ typedef u8 __bitwise blk_status_t;
>  
>  #define BLK_STS_AGAIN((__force blk_status_t)12)
>  
> +static inline bool blk_retryable(blk_status_t error)
> +{
> + switch (error) {
> + case BLK_STS_NOTSUPP:
> + case BLK_STS_NOSPC:
> + case BLK_STS_TARGET:
> + case BLK_STS_NEXUS:
> + case BLK_STS_MEDIUM:
> + case BLK_STS_PROTECTION:
> + return false;
> + }
> +
> + /* Anything else could be a path failure, so should be retried */
> + return true;
> +}
> +
>  struct blk_issue_stat {
>   u64 stat;
>  };
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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