[GIT PULL] nvme update for Linux 4.14, take 2

[GIT PULL] nvme update for Linux 4.14, take 2

[Christoph Hellwig]

Hi Jens,

below is the current set of NVMe updates for Linux 4.14, now against your
postmerge branch, and with three more patches.

The biggest bit comes from Sagi and refactors the RDMA driver to prepare
for more code sharing in the setup and teardown path.  But we have various
features and bug fixes from a lot of people as well.

The following changes since commit cd996fb47c360320cf25ac9503c16de085ea9cfc:

  Merge tag 'v4.13-rc7' into for-4.14/block-postmerge (2017-08-28 13:00:44 -0600)

are available in the git repository at:

  git://git.infradead.org/nvme.git nvme-4.14

for you to fetch changes up to 1d5df6af8c7469f9ae3e66e7bed0782cfe4f95db:

  nvme: don't blindly overwrite identifiers on disk revalidate (2017-08-29 10:23:04 +0200)

----------------------------------------------------------------
Arnav Dawn (2):
      nvme: add support for FW activation without reset
      nvme: define NVME_NSID_ALL

Christoph Hellwig (6):
      nvmet: use NVME_NSID_ALL
      nvme: report more detailed status codes to the block layer
      nvme: allow calling nvme_change_ctrl_state from irq context
      nvme: remove unused struct nvme_ns fields
      nvme: remove nvme_revalidate_ns
      nvme: don't blindly overwrite identifiers on disk revalidate

Guan Junxiong (2):
      nvmet: fix the return error code of target if host is not allowed
      nvme-fabrics: log a warning if hostid is invalid

James Smart (2):
      nvme-fc: Reattach to localports on re-registration
      nvmet-fc: simplify sg list handling

Jan H. Sch?nherr (1):
      nvme: fix uninitialized prp2 value on small transfers

Johannes Thumshirn (2):
      nvmet-fcloop: remove ALL_OPTS define
      nvme-rdma: remove NVME_RDMA_MAX_SEGMENT_SIZE

Jon Derrick (1):
      nvme: add support for NVMe 1.3 Timestamp Feature

Martin K. Petersen (1):
      nvme: honor RTD3 Entry Latency for shutdowns

Martin Wilck (2):
      string.h: add memcpy_and_pad()
      nvmet: use memcpy_and_pad for identify sn/fr

Max Gurtovoy (3):
      nvme: add symbolic constants for CC identifiers
      nvme: rename AMS symbolic constants to fit specification
      nvme-rdma: Use unlikely macro in the fast path

Sagi Grimberg (13):
      nvme-rdma: move nvme_rdma_configure_admin_queue code location
      nvme: Add admin_tagset pointer to nvme_ctrl
      nvme-rdma: move tagset allocation to a dedicated routine
      nvme-rdma: disable the controller on resets
      nvme-rdma: don't free tagset on resets
      nvme-rdma: reuse configure/destroy_admin_queue
      nvme-rdma: introduce configure/destroy io queues
      nvme-rdma: stop queues instead of simply flipping their state
      nvme-rdma: rename nvme_rdma_init_queue to nvme_rdma_alloc_queue
      nvme-rdma: introduce nvme_rdma_start_queue
      nvme-rdma: cleanup error path in controller reset
      nvme-rdma: call ops->reg_read64 instead of nvmf_reg_read64
      nvme: fix identify namespace logging

 drivers/nvme/host/core.c        | 242 +++++++++++++----
 drivers/nvme/host/fabrics.c     |   1 +
 drivers/nvme/host/fc.c          | 145 ++++++++---
 drivers/nvme/host/nvme.h        |  10 +-
 drivers/nvme/host/pci.c         |   5 +-
 drivers/nvme/host/rdma.c        | 564 ++++++++++++++++++++--------------------
 drivers/nvme/target/admin-cmd.c |  16 +-
 drivers/nvme/target/configfs.c  |   2 +-
 drivers/nvme/target/core.c      |  15 +-
 drivers/nvme/target/fc.c        |  48 +---
 drivers/nvme/target/fcloop.c    |   3 -
 drivers/nvme/target/loop.c      |   1 +
 include/linux/nvme-fc-driver.h  |   2 +-
 include/linux/nvme.h            |  37 ++-
 include/linux/string.h          |  30 +++
 15 files changed, 677 insertions(+), 444 deletions(-)

[GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/29/2017 09:05 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> below is the current set of NVMe updates for Linux 4.14, now against your
> postmerge branch, and with three more patches.
> 
> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
> for more code sharing in the setup and teardown path.  But we have various
> features and bug fixes from a lot of people as well.

Pulled, thanks.

-- 
Jens Axboe

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> Hi Jens,
>>
>> below is the current set of NVMe updates for Linux 4.14, now against your
>> postmerge branch, and with three more patches.
>>
>> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
>> for more code sharing in the setup and teardown path.  But we have various
>> features and bug fixes from a lot of people as well.
> 
> Pulled, thanks.

I just realized that patch:
--
commit d352ae205d8b05f3f7558d10f474d8436581b3e2
Author: Bart Van Assche <bart.vanassche at wdc.com>
Date:   Thu Aug 17 16:23:03 2017 -0700

     blk-mq: Make blk_mq_reinit_tagset() calls easier to read

     Since blk_mq_ops.reinit_request is only called from inside
     blk_mq_reinit_tagset(), make this function pointer an argument of
     blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
     This patch does not change any functionality but makes
     blk_mq_reinit_tagset() calls easier to read and to analyze.
--

Makes it impossible for me to move controller reset flow to
nvme-core without adding a trampoline (as the reinit_request
is transport specific)...

I'd really like to avoid adding more churn to the code refactoring
than it already has.

Given that this patch is not fixing anything should we revert it?

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@18:10 +0300, Sagi Grimberg wrote:
> I just realized that patch:
> --
> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
> Author: Bart Van Assche <bart.vanassche at wdc.com>
> Date:   Thu Aug 17 16:23:03 2017 -0700
> 
>      blk-mq: Make blk_mq_reinit_tagset() calls easier to read
> 
>      Since blk_mq_ops.reinit_request is only called from inside
>      blk_mq_reinit_tagset(), make this function pointer an argument of
>      blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>      This patch does not change any functionality but makes
>      blk_mq_reinit_tagset() calls easier to read and to analyze.
> --
> 
> Makes it impossible for me to move controller reset flow to
> nvme-core without adding a trampoline (as the reinit_request
> is transport specific)...

Hello Sagi,

Sorry but I doubt that that patch makes it "impossible" to move controller
reset flow to the NVMe core. There are already several function pointers in
the nvme_ctrl_ops data structure and there is one such data structure per
transport. Had you already considered to add a function pointer to that
structure?

Bart.

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> I just realized that patch:
>> --
>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>> Author: Bart Van Assche <bart.vanassche at wdc.com>
>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>
>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>
>>       Since blk_mq_ops.reinit_request is only called from inside
>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>       This patch does not change any functionality but makes
>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>> --
>>
>> Makes it impossible for me to move controller reset flow to
>> nvme-core without adding a trampoline (as the reinit_request
>> is transport specific)...
> 
> Hello Sagi,
> 
> Sorry but I doubt that that patch makes it "impossible" to move controller
> reset flow to the NVMe core. There are already several function pointers in
> the nvme_ctrl_ops data structure and there is one such data structure per
> transport. Had you already considered to add a function pointer to that
> structure?

I have, that's the trampoline function that I was referring to, it feels
a bit funny to have aa nvme core function that would look like:

int nvme_reinit_request()
{
	return ctrl->ops->reinit_request()
}

I can easily do that, but doesn't it defeat the purpose
of blk_mq_ops?

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@18:33 +0300, Sagi Grimberg wrote:
> > > I just realized that patch:
> > > --
> > > commit d352ae205d8b05f3f7558d10f474d8436581b3e2
> > > Author: Bart Van Assche <bart.vanassche at wdc.com>
> > > Date:   Thu Aug 17 16:23:03 2017 -0700
> > > 
> > >       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
> > > 
> > >       Since blk_mq_ops.reinit_request is only called from inside
> > >       blk_mq_reinit_tagset(), make this function pointer an argument of
> > >       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
> > >       This patch does not change any functionality but makes
> > >       blk_mq_reinit_tagset() calls easier to read and to analyze.
> > > --
> > > 
> > > Makes it impossible for me to move controller reset flow to
> > > nvme-core without adding a trampoline (as the reinit_request
> > > is transport specific)...
> > 
> > Hello Sagi,
> > 
> > Sorry but I doubt that that patch makes it "impossible" to move controller
> > reset flow to the NVMe core. There are already several function pointers in
> > the nvme_ctrl_ops data structure and there is one such data structure per
> > transport. Had you already considered to add a function pointer to that
> > structure?
> 
> I have, that's the trampoline function that I was referring to, it feels
> a bit funny to have aa nvme core function that would look like:
> 
> int nvme_reinit_request()
> {
> 	return ctrl->ops->reinit_request()
> }
> 
> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?

I don't think so. Request reinitialization is an NVMe concept that is not used
by any other block driver, so why should the pointer to the reinitialization
function exist in blk_mq_ops?

Bart.

[GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/30/2017 09:46 AM, Bart Van Assche wrote:
> On Wed, 2017-08-30@18:33 +0300, Sagi Grimberg wrote:
>>>> I just realized that patch:
>>>> --
>>>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>>>> Author: Bart Van Assche <bart.vanassche at wdc.com>
>>>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>>>
>>>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>>>
>>>>       Since blk_mq_ops.reinit_request is only called from inside
>>>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>>>       This patch does not change any functionality but makes
>>>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>>>> --
>>>>
>>>> Makes it impossible for me to move controller reset flow to
>>>> nvme-core without adding a trampoline (as the reinit_request
>>>> is transport specific)...
>>>
>>> Hello Sagi,
>>>
>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>> reset flow to the NVMe core. There are already several function pointers in
>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>> transport. Had you already considered to add a function pointer to that
>>> structure?
>>
>> I have, that's the trampoline function that I was referring to, it feels
>> a bit funny to have aa nvme core function that would look like:
>>
>> int nvme_reinit_request()
>> {
>> 	return ctrl->ops->reinit_request()
>> }
>>
>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
> 
> I don't think so. Request reinitialization is an NVMe concept that is
> not used by any other block driver, so why should the pointer to the
> reinitialization function exist in blk_mq_ops?

The point of blk-mq is to provide all the functionality that drivers
need, even if it is for just a single driver. Functionality that can be
removed from drivers is good. The smaller/simpler we can make the
driver, the better off we are, even if that means adding a bit of
complexity to the core.

Obviously this is a case-by-case decision. For this particular case, I'm
happy with either solution.

-- 
Jens Axboe

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>>>> Hello Sagi,
>>>>
>>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>>> reset flow to the NVMe core. There are already several function pointers in
>>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>>> transport. Had you already considered to add a function pointer to that
>>>> structure?
>>>
>>> I have, that's the trampoline function that I was referring to, it feels
>>> a bit funny to have aa nvme core function that would look like:
>>>
>>> int nvme_reinit_request()
>>> {
>>> 	return ctrl->ops->reinit_request()
>>> }
>>>
>>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
>>
>> I don't think so. Request reinitialization is an NVMe concept that is
>> not used by any other block driver, so why should the pointer to the
>> reinitialization function exist in blk_mq_ops?
> 
> The point of blk-mq is to provide all the functionality that drivers
> need, even if it is for just a single driver. Functionality that can be
> removed from drivers is good. The smaller/simpler we can make the
> driver, the better off we are, even if that means adding a bit of
> complexity to the core.
> 
> Obviously this is a case-by-case decision. For this particular case, I'm
> happy with either solution.

If we get to choose, my preference would be to restore the old behavior
because currently existing nvme transports keep their internal ctrl
representation in the tagset->driver_data as they implement
init/exit_request. Bouncing in nvme core would require to change that
and always keep struct nvme_ctrl as the tagset->driver_data and convert
it on every handler...

Everything is doable but it seems like an unneeded hassle to me...

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@19:05 +0300, Sagi Grimberg wrote:
> If we get to choose, my preference would be to restore the old behavior
> because currently existing nvme transports keep their internal ctrl
> representation in the tagset->driver_data as they implement
> init/exit_request. Bouncing in nvme core would require to change that
> and always keep struct nvme_ctrl as the tagset->driver_data and convert
> it on every handler...
> 
> Everything is doable but it seems like an unneeded hassle to me...

Sorry but I'm not convinced that it would be necessary to change what
tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
the block layer core to the NVMe core, to rename it and to pass a pointer
to the nvme_ctrl data structure to that function instead of only block layer
information?

Bart.

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> If we get to choose, my preference would be to restore the old behavior
>> because currently existing nvme transports keep their internal ctrl
>> representation in the tagset->driver_data as they implement
>> init/exit_request. Bouncing in nvme core would require to change that
>> and always keep struct nvme_ctrl as the tagset->driver_data and convert
>> it on every handler...
>>
>> Everything is doable but it seems like an unneeded hassle to me...
> 
> Sorry but I'm not convinced that it would be necessary to change what
> tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
> the block layer core to the NVMe core, to rename it and to pass a pointer
> to the nvme_ctrl data structure to that function instead of only block layer
> information?

That would mean that I need to open-code the tagset iteration in nvme
which does not feel like something a driver should do.

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@19:47 +0300, Sagi Grimberg wrote:
> > > If we get to choose, my preference would be to restore the old behavior
> > > because currently existing nvme transports keep their internal ctrl
> > > representation in the tagset->driver_data as they implement
> > > init/exit_request. Bouncing in nvme core would require to change that
> > > and always keep struct nvme_ctrl as the tagset->driver_data and convert
> > > it on every handler...
> > > 
> > > Everything is doable but it seems like an unneeded hassle to me...
> > 
> > Sorry but I'm not convinced that it would be necessary to change what
> > tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
> > the block layer core to the NVMe core, to rename it and to pass a pointer
> > to the nvme_ctrl data structure to that function instead of only block layer
> > information?
> 
> That would mean that I need to open-code the tagset iteration in nvme
> which does not feel like something a driver should do.

How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
to make the argument list of blk_mq_tagset_iter() more similar to that of
blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
can pass a pointer to any structure through the @priv argument? That would
make this function more general and maybe also more useful to other block
drivers.

Bart.

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> That would mean that I need to open-code the tagset iteration in nvme
>> which does not feel like something a driver should do.
> 
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

We could do that. But it feels like trying to go head over heals just to
keep a change titled:

"blk-mq: Make blk_mq_reinit_tagset() calls easier to read"

Which I'm not exactly sure I share the motivation. Also, I kinda liked
the symmetry of init/exit/reinit_request calling convention.

But, if you absolutely think its necessary to keep the change, we can
add an "all tags" iterator and use that to implement reinit_request in
nvme.

Christoph? Jens? verdict before we go forward here?

[GIT PULL] nvme update for Linux 4.14, take 2

[hch]

On Wed, Aug 30, 2017@04:56:28PM +0000, Bart Van Assche wrote:
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

I like that idea.  Even if it makes Sagi's work slightly harder :)

Can we get that into 4.14 still to have a good base?