Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Mike Christie]

On 07/02/2014 05:15 PM, Alex Williamson wrote:
> On Sat, 2014-06-28 at 10:47 -0700, Greg Kroah-Hartman wrote:
>> > 3.15-stable review patch.  If anyone has any objections, please let me know.
>> > 
>> > ------------------
>> > 
>> > From: Sagi Grimberg <sagig@mellanox.com>
>> > 
>> > commit d77e65350f2d82dfa0557707d505711f5a43c8fd upstream.
>> > 
>> > In case protection information exists over the wire
>> > iscsi header data length is required to include it.
>> > Use protection information aware scsi helpers to set
>> > the correct transfer length.
>> > 
>> > In order to avoid breakage, remove iser transfer length
>> > checks for each task as they are not always true and
>> > somewhat redundant anyway.
>> > 
>> > Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
>> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
>> > Acked-by: Mike Christie <michaelc@cs.wisc.edu>
>> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > 
>> > ---
>> >  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++--------------------
>> >  drivers/scsi/libiscsi.c                      |   18 +++++++-------
>> >  2 files changed, 19 insertions(+), 33 deletions(-)
> I'm a bit late to object, but this is causing a regression for me on
> 3.15.3 and 3.16-rc.  Using an iscsi_tcp rootfs, I'm seeing the NULL


A fix for this is in Christoph's tree. Here is the status of the patch.

http://marc.info/?l=linux-kernel&m=140432482527273&w=2

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Alex Williamson]

On Wed, 2014-07-02 at 17:44 -0500, Mike Christie wrote:
> On 07/02/2014 05:15 PM, Alex Williamson wrote:
> > On Sat, 2014-06-28 at 10:47 -0700, Greg Kroah-Hartman wrote:
> >> > 3.15-stable review patch.  If anyone has any objections, please let me know.
> >> > 
> >> > ------------------
> >> > 
> >> > From: Sagi Grimberg <sagig@mellanox.com>
> >> > 
> >> > commit d77e65350f2d82dfa0557707d505711f5a43c8fd upstream.
> >> > 
> >> > In case protection information exists over the wire
> >> > iscsi header data length is required to include it.
> >> > Use protection information aware scsi helpers to set
> >> > the correct transfer length.
> >> > 
> >> > In order to avoid breakage, remove iser transfer length
> >> > checks for each task as they are not always true and
> >> > somewhat redundant anyway.
> >> > 
> >> > Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> >> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
> >> > Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> >> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > 
> >> > ---
> >> >  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++--------------------
> >> >  drivers/scsi/libiscsi.c                      |   18 +++++++-------
> >> >  2 files changed, 19 insertions(+), 33 deletions(-)
> > I'm a bit late to object, but this is causing a regression for me on
> > 3.15.3 and 3.16-rc.  Using an iscsi_tcp rootfs, I'm seeing the NULL
> 
> 
> A fix for this is in Christoph's tree. Here is the status of the patch.
> 
> http://marc.info/?l=linux-kernel&m=140432482527273&w=2

Thanks for the pointer.  It's curious why there was no objection to this
going into stable if it's been known to be broken for a week.

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[James Bottomley]

On Wed, 2014-07-02 at 17:44 -0500, Mike Christie wrote:
> On 07/02/2014 05:15 PM, Alex Williamson wrote:
> > On Sat, 2014-06-28 at 10:47 -0700, Greg Kroah-Hartman wrote:
> >> > 3.15-stable review patch.  If anyone has any objections, please let me know.
> >> > 
> >> > ------------------
> >> > 
> >> > From: Sagi Grimberg <sagig@mellanox.com>
> >> > 
> >> > commit d77e65350f2d82dfa0557707d505711f5a43c8fd upstream.
> >> > 
> >> > In case protection information exists over the wire
> >> > iscsi header data length is required to include it.
> >> > Use protection information aware scsi helpers to set
> >> > the correct transfer length.
> >> > 
> >> > In order to avoid breakage, remove iser transfer length
> >> > checks for each task as they are not always true and
> >> > somewhat redundant anyway.
> >> > 
> >> > Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> >> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
> >> > Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> >> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > 
> >> > ---
> >> >  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++--------------------
> >> >  drivers/scsi/libiscsi.c                      |   18 +++++++-------
> >> >  2 files changed, 19 insertions(+), 33 deletions(-)
> > I'm a bit late to object, but this is causing a regression for me on
> > 3.15.3 and 3.16-rc.  Using an iscsi_tcp rootfs, I'm seeing the NULL
> 
> 
> A fix for this is in Christoph's tree. Here is the status of the patch.
> 
> http://marc.info/?l=linux-kernel&m=140432482527273&w=2

I assume it's this fix:

commit 7bafc240e126c37f60b040fa429ae4a710b88ab5
Author: Martin K. Petersen <martin.petersen@oracle.com>
Date:   Tue Jun 24 16:59:35 2014 +0200

    scsi: use the scsi data buffer length to extract transfer size
 
In which case it's not marked for stable backport ... I assume it should
be?

James

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Alex Williamson]

On Wed, 2014-07-02 at 15:52 -0700, James Bottomley wrote:
> On Wed, 2014-07-02 at 17:44 -0500, Mike Christie wrote:
> > On 07/02/2014 05:15 PM, Alex Williamson wrote:
> > > On Sat, 2014-06-28 at 10:47 -0700, Greg Kroah-Hartman wrote:
> > >> > 3.15-stable review patch.  If anyone has any objections, please let me know.
> > >> > 
> > >> > ------------------
> > >> > 
> > >> > From: Sagi Grimberg <sagig@mellanox.com>
> > >> > 
> > >> > commit d77e65350f2d82dfa0557707d505711f5a43c8fd upstream.
> > >> > 
> > >> > In case protection information exists over the wire
> > >> > iscsi header data length is required to include it.
> > >> > Use protection information aware scsi helpers to set
> > >> > the correct transfer length.
> > >> > 
> > >> > In order to avoid breakage, remove iser transfer length
> > >> > checks for each task as they are not always true and
> > >> > somewhat redundant anyway.
> > >> > 
> > >> > Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> > >> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
> > >> > Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> > >> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > >> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >> > 
> > >> > ---
> > >> >  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++--------------------
> > >> >  drivers/scsi/libiscsi.c                      |   18 +++++++-------
> > >> >  2 files changed, 19 insertions(+), 33 deletions(-)
> > > I'm a bit late to object, but this is causing a regression for me on
> > > 3.15.3 and 3.16-rc.  Using an iscsi_tcp rootfs, I'm seeing the NULL
> > 
> > 
> > A fix for this is in Christoph's tree. Here is the status of the patch.
> > 
> > http://marc.info/?l=linux-kernel&m=140432482527273&w=2
> 
> I assume it's this fix:
> 
> commit 7bafc240e126c37f60b040fa429ae4a710b88ab5
> Author: Martin K. Petersen <martin.petersen@oracle.com>
> Date:   Tue Jun 24 16:59:35 2014 +0200
> 
>     scsi: use the scsi data buffer length to extract transfer size
>  
> In which case it's not marked for stable backport ... I assume it should
> be?

Yes, that's the one.  It definitely needs a 3.15 stable backport.
Thanks,

Alex

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Nicholas A. Bellinger]

On Wed, 2014-07-02 at 17:19 -0600, Alex Williamson wrote:
> On Wed, 2014-07-02 at 15:52 -0700, James Bottomley wrote:
> > On Wed, 2014-07-02 at 17:44 -0500, Mike Christie wrote:
> > > On 07/02/2014 05:15 PM, Alex Williamson wrote:
> > > > On Sat, 2014-06-28 at 10:47 -0700, Greg Kroah-Hartman wrote:
> > > >> > 3.15-stable review patch.  If anyone has any objections, please let me know.
> > > >> > 
> > > >> > ------------------
> > > >> > 
> > > >> > From: Sagi Grimberg <sagig@mellanox.com>
> > > >> > 
> > > >> > commit d77e65350f2d82dfa0557707d505711f5a43c8fd upstream.
> > > >> > 
> > > >> > In case protection information exists over the wire
> > > >> > iscsi header data length is required to include it.
> > > >> > Use protection information aware scsi helpers to set
> > > >> > the correct transfer length.
> > > >> > 
> > > >> > In order to avoid breakage, remove iser transfer length
> > > >> > checks for each task as they are not always true and
> > > >> > somewhat redundant anyway.
> > > >> > 
> > > >> > Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> > > >> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
> > > >> > Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> > > >> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > >> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >> > 
> > > >> > ---
> > > >> >  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++--------------------
> > > >> >  drivers/scsi/libiscsi.c                      |   18 +++++++-------
> > > >> >  2 files changed, 19 insertions(+), 33 deletions(-)
> > > > I'm a bit late to object, but this is causing a regression for me on
> > > > 3.15.3 and 3.16-rc.  Using an iscsi_tcp rootfs, I'm seeing the NULL
> > > 
> > > 
> > > A fix for this is in Christoph's tree. Here is the status of the patch.
> > > 
> > > http://marc.info/?l=linux-kernel&m=140432482527273&w=2
> > 
> > I assume it's this fix:
> > 
> > commit 7bafc240e126c37f60b040fa429ae4a710b88ab5
> > Author: Martin K. Petersen <martin.petersen@oracle.com>
> > Date:   Tue Jun 24 16:59:35 2014 +0200
> > 
> >     scsi: use the scsi data buffer length to extract transfer size
> >  
> > In which case it's not marked for stable backport ... I assume it should
> > be?
> 
> Yes, that's the one.  It definitely needs a 3.15 stable backport.
> Thanks,
> 

Yes.  James, please mark this for v3.15.y and push to Linus ASAP.

Thank you,

--nab

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Christoph Hellwig]

On Wed, Jul 02, 2014 at 04:48:37PM -0600, Alex Williamson wrote:
> Thanks for the pointer.  It's curious why there was no objection to this
> going into stable if it's been known to be broken for a week.

I've never seen the stable patch on linux-scsi.  IMHO it should never
have been proposed for -stable with all the issues it caused.

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Christoph Hellwig]

On Wed, Jul 02, 2014 at 03:52:35PM -0700, James Bottomley wrote:
> In which case it's not marked for stable backport ... I assume it should
> be?

Only if the patch that regression went to stable, which it shouldn't,

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[James Bottomley]

On Wed, 2014-07-02 at 23:05 -0700, Christoph Hellwig wrote:
> On Wed, Jul 02, 2014 at 03:52:35PM -0700, James Bottomley wrote:
> > In which case it's not marked for stable backport ... I assume it should
> > be?
> 
> Only if the patch that regression went to stable, which it shouldn't,

Well it did:

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/drivers/scsi?h=linux-3.15.y&id=04423ddea30a7fb7232636eda8aed55ea5b972fe

The stable process is a bit broken from my point of view since it
doesn't copy linux-scsi on SCSI commits ... but this time even I didn't
get a cc, so really no-one who knew there was a problem had a chance to
comment.

The choice is either request revert from stable or add fix.

James

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Greg Kroah-Hartman]

On Thu, Jul 03, 2014 at 06:54:05AM -0700, James Bottomley wrote:
> On Wed, 2014-07-02 at 23:05 -0700, Christoph Hellwig wrote:
> > On Wed, Jul 02, 2014 at 03:52:35PM -0700, James Bottomley wrote:
> > > In which case it's not marked for stable backport ... I assume it should
> > > be?
> > 
> > Only if the patch that regression went to stable, which it shouldn't,
> 
> Well it did:
> 
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/drivers/scsi?h=linux-3.15.y&id=04423ddea30a7fb7232636eda8aed55ea5b972fe
> 
> The stable process is a bit broken from my point of view since it
> doesn't copy linux-scsi on SCSI commits

I copy everyone on the patch.  You were not on that patch as it doesn't
look like it went through your tree at all.

> ... but this time even I didn't get a cc, so really no-one who knew
> there was a problem had a chance to comment.

The target people (Nicholas) asked for the patch to be applied, they
knew what was going on here.

> The choice is either request revert from stable or add fix.

Agreed, just let me know.

greg k-h

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Christoph Hellwig]

On Thu, Jul 03, 2014 at 06:54:05AM -0700, James Bottomley wrote:
> Well it did:
> 
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/drivers/scsi?h=linux-3.15.y&id=04423ddea30a7fb7232636eda8aed55ea5b972fe
> 
> The stable process is a bit broken from my point of view since it
> doesn't copy linux-scsi on SCSI commits ... but this time even I didn't
> get a cc, so really no-one who knew there was a problem had a chance to
> comment.
> 
> The choice is either request revert from stable or add fix.

Unfortunately the fix hasn't even made it to mainline yet, despite lots
of people tripping over it.  If you're too busy I'm happy to send it to
Linus, it's been sitting in my tree for over a week now.

So if we can get it in ASAP we can also send it to stable, otherwise
a revert is the only option.

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Christoph Hellwig]

On Thu, Jul 03, 2014 at 09:27:48AM -0700, Greg Kroah-Hartman wrote:
> I copy everyone on the patch.  You were not on that patch as it doesn't
> look like it went through your tree at all.

It went through the target tree despite exclusive touching scsi
initiator side code.  I'm still not sure how this happened, but we
should take care to avoid this in the future.  In addition to this
regression due to a complete lack of testing it also caused various
merge issues.

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[James Bottomley]

On Thu, 2014-07-03 at 09:33 -0700, Christoph Hellwig wrote:
> On Thu, Jul 03, 2014 at 09:27:48AM -0700, Greg Kroah-Hartman wrote:
> > I copy everyone on the patch.  You were not on that patch as it doesn't
> > look like it went through your tree at all.
> 
> It went through the target tree despite exclusive touching scsi
> initiator side code.  I'm still not sure how this happened, but we
> should take care to avoid this in the future.  In addition to this
> regression due to a complete lack of testing it also caused various
> merge issues.

OK, Nic cc'd

Please explain how this commit:

commit d77e65350f2d82dfa0557707d505711f5a43c8fd
Author: Sagi Grimberg <sagig@mellanox.com>
Date:   Wed Jun 11 12:09:58 2014 +0300

    libiscsi, iser: Adjust data_length to include protection information
    
    In case protection information exists over the wire
    iscsi header data length is required to include it.
    Use protection information aware scsi helpers to set
    the correct transfer length.
    
    In order to avoid breakage, remove iser transfer length
    checks for each task as they are not always true and
    somewhat redundant anyway.
    
    Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
    Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
    Acked-by: Mike Christie <michaelc@cs.wisc.edu>
    Cc: stable@vger.kernel.org # 3.15+
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

 drivers/infiniband/ulp/iser/iser_initiator.c | 34 ++++++++--------------------
 drivers/scsi/libiscsi.c                      | 18 +++++++--------

Came to go through the target tree even though it's initiator only and
how come it was tagged for stable?

James

Re: [PATCH 3.15 099/139] libiscsi, iser: Adjust data_length to include protection information

[Sagi Grimberg]

On 7/3/2014 9:00 PM, James Bottomley wrote:
> On Thu, 2014-07-03 at 09:33 -0700, Christoph Hellwig wrote:
>> On Thu, Jul 03, 2014 at 09:27:48AM -0700, Greg Kroah-Hartman wrote:
>>> I copy everyone on the patch.  You were not on that patch as it doesn't
>>> look like it went through your tree at all.
>> It went through the target tree despite exclusive touching scsi
>> initiator side code.  I'm still not sure how this happened, but we
>> should take care to avoid this in the future.  In addition to this
>> regression due to a complete lack of testing it also caused various
>> merge issues.
> OK, Nic cc'd
>
> Please explain how this commit:
>
> commit d77e65350f2d82dfa0557707d505711f5a43c8fd
> Author: Sagi Grimberg <sagig@mellanox.com>
> Date:   Wed Jun 11 12:09:58 2014 +0300
>
>      libiscsi, iser: Adjust data_length to include protection information
>      
>      In case protection information exists over the wire
>      iscsi header data length is required to include it.
>      Use protection information aware scsi helpers to set
>      the correct transfer length.
>      
>      In order to avoid breakage, remove iser transfer length
>      checks for each task as they are not always true and
>      somewhat redundant anyway.
>      
>      Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
>      Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
>      Acked-by: Mike Christie <michaelc@cs.wisc.edu>
>      Cc: stable@vger.kernel.org # 3.15+
>      Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>
>   drivers/infiniband/ulp/iser/iser_initiator.c | 34 ++++++++--------------------
>   drivers/scsi/libiscsi.c                      | 18 +++++++--------
>
> Came to go through the target tree even though it's initiator only and
> how come it was tagged for stable?

Hey James,

I think it's because of the cover-letter comment I gave in my initial 
patchset:
"Although these patches involve 3 subsystems with different maintainers 
(scsi, iser, target) I would prefer seeing these patches included together."
The set involved initiator-target wire protocol dependency.

I removed this comment in v1, v2 due to Roland's comment:
"Why? Because they break wire compatibility? I hate to say it but even 
if they're merged at the same time,
you can't guarantee that targets and initiators will be updated together."

So I guess Nic just followed up on my request in order to avoid 
wire-protocol breakage.
I assume the stable tag was there for the same reason.

Anyway, Sorry for all the fuss... I'll try to avoid such mistakes in the 
future.

Sagi.