Re: [PATCH v2] nvme-fabrics: reject I/O to offline device

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Feb 03 2020 at  8:40am -0500,
Victor Gladkov <Victor.Gladkov@kioxia.com> wrote:

> On Sun, Jan 30, 2020 at 17:09, Christoph Hellwig wrote:
> > On Sun, Jan 26, 2020 at 10:06:20AM +0000, Victor Gladkov wrote:
> > > On 1/15/2020 5:43 PM, Victor Gladkov wrote:
> > > > 1. Added multipath support for this patch.
> > > > 2. Small refactoring (according to the review)
> > >
> > > Anyone have any comments on the latest proposed patch?
> > 
> > Where is the latest patch?  I didn't see a repost.  To kick off a discussion
> > please just resent it, in proper patch format and with the proper style to make
> > reviewing it easier.
> 
> Patch updated for branch nvme/for-5.6

Christoph meant you should send a patch with a version bump in subject
(e.g. [PATCH v3]) and a proper patch header. No maintainer can pick this
patch up like you've posted (not without forcing them to do more work
than they should).

Some nits below.

> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index a0ec40a..fd8c7dd 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -15,6 +15,8 @@
>  #define NVMF_DEF_RECONNECT_DELAY	10
>  /* default to 600 seconds of reconnect attempts before giving up */
>  #define NVMF_DEF_CTRL_LOSS_TMO		600
> +/* default to 30 seconds of fail fast IO commands  */
> +#define NVMF_DEF_FAIL_FAST_TMO		0

Your comment says "default to 30 seconds" yet in practice 0 is used?

>  /*
>   * Define a host as seen by the target.  We allocate one at boot, but also
> @@ -56,6 +58,7 @@ enum {
>  	NVMF_OPT_NR_WRITE_QUEUES = 1 << 17,
>  	NVMF_OPT_NR_POLL_QUEUES = 1 << 18,
>  	NVMF_OPT_TOS		= 1 << 19,
> +	NVMF_OPT_FAIL_FAST_TMO	= 1 << 20,
>  };
> 
>  /**
> @@ -89,6 +92,7 @@ enum {
>   * @nr_write_queues: number of queues for write I/O
>   * @nr_poll_queues: number of queues for polling I/O
>   * @tos: type of service
> + * @fast_fail_tmo_ns: Fast I/O fail timeout in nanoseconds;
>   */
>  struct nvmf_ctrl_options {
>  	unsigned		mask;
> @@ -111,6 +115,7 @@ struct nvmf_ctrl_options {
>  	unsigned int		nr_write_queues;
>  	unsigned int		nr_poll_queues;
>  	int			tos;
> +	unsigned int	fail_fast_tmo;
>  };

Above you document "@fast_fail_tmo_ns" but then add the member with name
"fail_fast_tmo".

>  /*
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 797c183..4edcaf1 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -281,9 +281,11 @@ static bool nvme_available_path(struct nvme_ns_head *head)
> 
>  	list_for_each_entry_rcu(ns, &head->list, siblings) {
>  		switch (ns->ctrl->state) {
> +		case NVME_CTRL_CONNECTING:
> +		    if(test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ns->ctrl->flags))
> +				break;

James suggested this case order, when he did he included /* fallthru */,
it helps to have it where appropriate (like this case).

>  		case NVME_CTRL_LIVE:
>  		case NVME_CTRL_RESETTING:
> -		case NVME_CTRL_CONNECTING:
>  			/* fallthru */
>  			return true;
>  		default:
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1024fec..b6a199e 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -256,6 +256,7 @@ struct nvme_ctrl {
>  	struct work_struct scan_work;
>  	struct work_struct async_event_work;
>  	struct delayed_work ka_work;
> +	struct delayed_work failfast_work;
>  	struct nvme_command ka_cmd;
>  	struct work_struct fw_act_work;
>  	unsigned long events;
> @@ -289,6 +290,8 @@ struct nvme_ctrl {
>  	u16 icdoff;
>  	u16 maxcmd;
>  	int nr_reconnects;
> +	unsigned long flags;
> +#define NVME_CTRL_FAILFAST_EXPIRED	0
>  	struct nvmf_ctrl_options *opts;
> 
>  	struct page *discard_page;

Seems awkward to define bits for "flags" within the body of the struct;
but I'll defer to others' judgement.

Mike


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme