[PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Dakshaja Uppalapati]

This reverts upstream 'commit 530436c45ef2
("nvme: Discard workaround for non-conformant devices")'

Since commit `530436c45ef2` introduced a regression due to which
blk_update_request IO error is observed on formatting device, reverting it.

Fixes: 530436c45ef2 ("nvme: Discard workaround for non-conformant devices")
Cc: stable <stable@vger.kernel.org> # 4.19+
Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
---
 drivers/nvme/host/core.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f3c037f5a9ba..ec598a86f88e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -607,14 +607,8 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 	struct nvme_dsm_range *range;
 	struct bio *bio;
 
-	/*
-	 * Some devices do not consider the DSM 'Number of Ranges' field when
-	 * determining how much data to DMA. Always allocate memory for maximum
-	 * number of segments to prevent device reading beyond end of buffer.
-	 */
-	static const size_t alloc_size = sizeof(*range) * NVME_DSM_MAX_RANGES;
-
-	range = kzalloc(alloc_size, GFP_ATOMIC | __GFP_NOWARN);
+	range = kmalloc_array(segments, sizeof(*range),
+			      GFP_ATOMIC | __GFP_NOWARN);
 	if (!range) {
 		/*
 		 * If we fail allocation our range, fallback to the controller
@@ -654,7 +648,7 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 
 	req->special_vec.bv_page = virt_to_page(range);
 	req->special_vec.bv_offset = offset_in_page(range);
-	req->special_vec.bv_len = alloc_size;
+	req->special_vec.bv_len = sizeof(*range) * segments;
 	req->rq_flags |= RQF_SPECIAL_PAYLOAD;
 
 	return BLK_STS_OK;
-- 
2.18.0.232.gb7bd9486b.dirty

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Christoph Hellwig]

On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> This reverts upstream 'commit 530436c45ef2
> ("nvme: Discard workaround for non-conformant devices")'
> 
> Since commit `530436c45ef2` introduced a regression due to which
> blk_update_request IO error is observed on formatting device, reverting it.

Err, why?  Please send an actual bug report with details of your
setup.

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Dakshaja Uppalapati]

On Wednesday, June 06/03/20, 2020 at 15:07:50 +0200, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> > This reverts upstream 'commit 530436c45ef2
> > ("nvme: Discard workaround for non-conformant devices")'
> > 
> > Since commit `530436c45ef2` introduced a regression due to which
> > blk_update_request IO error is observed on formatting device, reverting it.
> 
> Err, why?  Please send an actual bug report with details of your
> setup.

Hi Christoph,

Here is the link describing the issue initially reported for upstream 
kernel 5.5:

https://lore.kernel.org/linux-nvme/CH2PR12MB40053A64681EFA3E6F63FDFBDD2A0@CH2PR12MB4005.namprd12.prod.outlook.com/

Issue is later fixed with upstream commit b716e688.

Now the same issue is observed with stable kernels 4.19 and 5.4 as 
commit 530436c4 was pulled in to stable. Here is the link describing the same 
for stable kernels.

https://www.spinics.net/lists/stable/msg387744.html

Please let me know if you need any further info.

Thanks,
Dakshaja

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Christoph Hellwig]

On Wed, Jun 03, 2020 at 09:47:18PM +0530, Dakshaja Uppalapati wrote:
> > Err, why?  Please send an actual bug report with details of your
> > setup.
> 
> Hi Christoph,
> 
> Here is the link describing the issue initially reported for upstream 
> kernel 5.5:
> 
> https://lore.kernel.org/linux-nvme/CH2PR12MB40053A64681EFA3E6F63FDFBDD2A0@CH2PR12MB4005.namprd12.prod.outlook.com/
> 
> Issue is later fixed with upstream commit b716e688.

We are talking about two different things here.  One is the Linux NVMe
host code that can be used with lots of different controllers.  Many of
them are PCIe controller, especially cheap ones.

The other is the Linux NVMe target code.  So if a fix for very common
PCIe controller trigger a bug in the target code there is no 1:1
relationship as even if you are talking to a Linux fabrics controller
it usually runs a different kernel version on a different system.

That being said you can always backport that fix as well, which probably
is a good idea as it fixes a real bug.

Nevermind that nothing in your revert patch indicated it wasn't for
mainline.

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Sagi Grimberg]

>>> Err, why?  Please send an actual bug report with details of your
>>> setup.
>>
>> Hi Christoph,
>>
>> Here is the link describing the issue initially reported for upstream
>> kernel 5.5:
>>
>> https://lore.kernel.org/linux-nvme/CH2PR12MB40053A64681EFA3E6F63FDFBDD2A0@CH2PR12MB4005.namprd12.prod.outlook.com/
>>
>> Issue is later fixed with upstream commit b716e688.
> 
> We are talking about two different things here.  One is the Linux NVMe
> host code that can be used with lots of different controllers.  Many of
> them are PCIe controller, especially cheap ones.
> 
> The other is the Linux NVMe target code.  So if a fix for very common
> PCIe controller trigger a bug in the target code there is no 1:1
> relationship as even if you are talking to a Linux fabrics controller
> it usually runs a different kernel version on a different system.
> 
> That being said you can always backport that fix as well, which probably
> is a good idea as it fixes a real bug.
> 
> Nevermind that nothing in your revert patch indicated it wasn't for
> mainline.

Agree..

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Dakshaja Uppalapati]

On Wednesday, June 06/03/20, 2020 at 14:20:01 -0700, Sagi Grimberg wrote:
> 
> > > > Err, why?  Please send an actual bug report with details of your
> > > > setup.
> > > 
> > > Hi Christoph,
> > > 
> > > Here is the link describing the issue initially reported for upstream
> > > kernel 5.5:
> > > 
> > > https://lore.kernel.org/linux-nvme/CH2PR12MB40053A64681EFA3E6F63FDFBDD2A0@CH2PR12MB4005.namprd12.prod.outlook.com/
> > > 
> > > Issue is later fixed with upstream commit b716e688.
> > 
> > We are talking about two different things here.  One is the Linux NVMe
> > host code that can be used with lots of different controllers.  Many of
> > them are PCIe controller, especially cheap ones.
> > 
> > The other is the Linux NVMe target code.  So if a fix for very common
> > PCIe controller trigger a bug in the target code there is no 1:1
> > relationship as even if you are talking to a Linux fabrics controller
> > it usually runs a different kernel version on a different system.
> > 
> > That being said you can always backport that fix as well, which probably
> > is a good idea as it fixes a real bug.
> > 
> > Nevermind that nothing in your revert patch indicated it wasn't for
> > mainline.
> 
> Agree..

Just to confirm that I got it right, Do you want me to send all 6 patches 
(fix and dependent patches) to stable?

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Greg KH]

On Thu, Jun 04, 2020 at 12:06:39PM +0530, Dakshaja Uppalapati wrote:
> On Wednesday, June 06/03/20, 2020 at 14:20:01 -0700, Sagi Grimberg wrote:
> > 
> > > > > Err, why?  Please send an actual bug report with details of your
> > > > > setup.
> > > > 
> > > > Hi Christoph,
> > > > 
> > > > Here is the link describing the issue initially reported for upstream
> > > > kernel 5.5:
> > > > 
> > > > https://lore.kernel.org/linux-nvme/CH2PR12MB40053A64681EFA3E6F63FDFBDD2A0@CH2PR12MB4005.namprd12.prod.outlook.com/
> > > > 
> > > > Issue is later fixed with upstream commit b716e688.
> > > 
> > > We are talking about two different things here.  One is the Linux NVMe
> > > host code that can be used with lots of different controllers.  Many of
> > > them are PCIe controller, especially cheap ones.
> > > 
> > > The other is the Linux NVMe target code.  So if a fix for very common
> > > PCIe controller trigger a bug in the target code there is no 1:1
> > > relationship as even if you are talking to a Linux fabrics controller
> > > it usually runs a different kernel version on a different system.
> > > 
> > > That being said you can always backport that fix as well, which probably
> > > is a good idea as it fixes a real bug.
> > > 
> > > Nevermind that nothing in your revert patch indicated it wasn't for
> > > mainline.
> > 
> > Agree..
> 
> Just to confirm that I got it right, Do you want me to send all 6 patches 
> (fix and dependent patches) to stable?

Yes, in a format that can be applied, thanks.

greg k-h

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Greg KH]

On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> This reverts upstream 'commit 530436c45ef2
> ("nvme: Discard workaround for non-conformant devices")'
> 
> Since commit `530436c45ef2` introduced a regression due to which
> blk_update_request IO error is observed on formatting device, reverting it.
> 
> Fixes: 530436c45ef2 ("nvme: Discard workaround for non-conformant devices")
> Cc: stable <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
> ---
>  drivers/nvme/host/core.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)

This was only for stable?

Totally confused...

greg k-h

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Dakshaja Uppalapati]

On Friday, June 06/05/20, 2020 at 15:43:40 +0200, Greg KH wrote:
> On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> > This reverts upstream 'commit 530436c45ef2
> > ("nvme: Discard workaround for non-conformant devices")'
> > 
> > Since commit `530436c45ef2` introduced a regression due to which
> > blk_update_request IO error is observed on formatting device, reverting it.
> > 
> > Fixes: 530436c45ef2 ("nvme: Discard workaround for non-conformant devices")
> > Cc: stable <stable@vger.kernel.org> # 4.19+
> > Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
> > ---
> >  drivers/nvme/host/core.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> This was only for stable?
> 

Yes this is for stable 5.4 and 4.19 branches

> Totally confused...

sorry for the confusion. Please let me know if I missed any detail.

Thanks,
Dakshaja

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Keith Busch]

On Mon, Jun 08, 2020 at 12:05:44PM +0530, Dakshaja Uppalapati wrote:
> On Friday, June 06/05/20, 2020 at 15:43:40 +0200, Greg KH wrote:
> > On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> > > This reverts upstream 'commit 530436c45ef2
> > > ("nvme: Discard workaround for non-conformant devices")'
> > > 
> > > Since commit `530436c45ef2` introduced a regression due to which
> > > blk_update_request IO error is observed on formatting device, reverting it.
> > > 
> > > Fixes: 530436c45ef2 ("nvme: Discard workaround for non-conformant devices")
> > > Cc: stable <stable@vger.kernel.org> # 4.19+
> > > Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
> > > ---
> > >  drivers/nvme/host/core.c | 12 +++---------
> > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > This was only for stable?
> > 
> 
> Yes this is for stable 5.4 and 4.19 branches

No, this revert needs to be rejected. The stable patches need to be the
upstream equivalent.

Re: [PATCH nvme] nvme: Revert "nvme: Discard workaround for non-conformant devices"

[Dakshaja Uppalapati]

On Tuesday, June 06/09/20, 2020 at 00:05:33 +0900, Keith Busch wrote:
> On Mon, Jun 08, 2020 at 12:05:44PM +0530, Dakshaja Uppalapati wrote:
> > On Friday, June 06/05/20, 2020 at 15:43:40 +0200, Greg KH wrote:
> > > On Wed, Jun 03, 2020 at 02:48:51PM +0530, Dakshaja Uppalapati wrote:
> > > > This reverts upstream 'commit 530436c45ef2
> > > > ("nvme: Discard workaround for non-conformant devices")'
> > > > 
> > > > Since commit `530436c45ef2` introduced a regression due to which
> > > > blk_update_request IO error is observed on formatting device, reverting it.
> > > > 
> > > > Fixes: 530436c45ef2 ("nvme: Discard workaround for non-conformant devices")
> > > > Cc: stable <stable@vger.kernel.org> # 4.19+
> > > > Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
> > > > ---
> > > >  drivers/nvme/host/core.c | 12 +++---------
> > > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > 
> > > This was only for stable?
> > > 
> > 
> > Yes this is for stable 5.4 and 4.19 branches
> 
> No, this revert needs to be rejected. The stable patches need to be the
> upstream equivalent.

Hi keith,

I have sent the required patches which are need to pulled from upstream to
stable 5.4 and 4.19 branches.

Please let me know if you need any further info.

Thanks,
Dakshaja