Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1388409 > unrolled thread

[PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Ben Hutchings <ben@decadent.org.uk> - 2016-04-27 02:10 +0200
    Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Rafael David Tinoco <rafael.tinoco@canonical.com> - 2016-04-27 22:50 +0200
      Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-04-28 02:10 +0200
      Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Ben Hutchings <ben@decadent.org.uk> - 2016-04-28 18:20 +0200
        Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Rafael David Tinoco <rafael.tinoco@canonical.com> - 2016-04-29 06:10 +0200
          Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-04-29 14:20 +0200
            Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Rafael David Tinoco <rafael.tinoco@canonical.com> - 2016-04-29 17:20 +0200
          Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Ben Hutchings <ben@decadent.org.uk> - 2016-04-30 21:40 +0200
            Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-05-03 03:50 +0200
          Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP Paolo Bonzini <pbonzini@redhat.com> - 2016-05-09 16:20 +0200

#1388409 — [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP

3.16.35-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: "Martin K. Petersen" <martin.petersen@oracle.com>

commit 7985090aa0201fa7760583f9f8e6ba41a8d4c392 upstream.

The T10 SBC UNMAP command does not provide any hard guarantees that
blocks will return zeroes on a subsequent READ. This is due to the fact
that the device server is free to silently ignore all or parts of the
request.

The only way to ensure that a block consistently returns zeroes after
being unmapped is to use WRITE SAME with the UNMAP bit set. Should the
device be unable to unmap one or more blocks described by the command it
is required to manually write zeroes to them.

Until now we have preferred UNMAP over the WRITE SAME variants to
accommodate thinly provisioned devices that predated the final SBC-3
spec. This patch changes the heuristic so that we favor WRITE SAME(16)
or (10) over UNMAP if these commands are marked as supported in the
Logical Block Provisioning VPD page.

The patch also disables discard_zeroes_data for devices operating in
UNMAP mode.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/scsi/sd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -627,7 +627,7 @@ static void sd_config_discard(struct scs
 	unsigned int logical_block_size = sdkp->device->sector_size;
 	unsigned int max_blocks = 0;
 
-	q->limits.discard_zeroes_data = sdkp->lbprz;
+	q->limits.discard_zeroes_data = 0;
 	q->limits.discard_alignment = sdkp->unmap_alignment *
 		logical_block_size;
 	q->limits.discard_granularity =
@@ -651,11 +651,13 @@ static void sd_config_discard(struct scs
 	case SD_LBP_WS16:
 		max_blocks = min_not_zero(sdkp->max_ws_blocks,
 					  (u32)SD_MAX_WS16_BLOCKS);
+		q->limits.discard_zeroes_data = sdkp->lbprz;
 		break;
 
 	case SD_LBP_WS10:
 		max_blocks = min_not_zero(sdkp->max_ws_blocks,
 					  (u32)SD_MAX_WS10_BLOCKS);
+		q->limits.discard_zeroes_data = sdkp->lbprz;
 		break;
 
 	case SD_LBP_ZERO:
@@ -2572,12 +2574,12 @@ static void sd_read_block_limits(struct
 
 		} else {	/* LBP VPD page tells us what to use */
 
-			if (sdkp->lbpu && sdkp->max_unmap_blocks)
-				sd_config_discard(sdkp, SD_LBP_UNMAP);
-			else if (sdkp->lbpws)
+			if (sdkp->lbpws)
 				sd_config_discard(sdkp, SD_LBP_WS16);
 			else if (sdkp->lbpws10)
 				sd_config_discard(sdkp, SD_LBP_WS10);
+			else if (sdkp->lbpu && sdkp->max_unmap_blocks)
+				sd_config_discard(sdkp, SD_LBP_UNMAP);
 			else
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
 		}

[toc] | [next] | [standalone]

#1389578

It seems that changing discard method from UNMAP to WRITE SAME(16)
without using NDOB bit (as first described in sbc3r35b.pdf) can cause
performance problems on big discards (since data-out buffer will be
checked for every WRITE SAME command). I think this is happening after
this commit, since NDOB bit wasn't implemented with this change
(afaik, iirc).

From the spec:
"""
To ensure that subsequent read operations return all zeros in a
logical block, use the WRITE SAME (16)
command with the NDOB bit set to one. If the UNMAP bit is set to one,
then the device server may unmap the logical blocks specified by the
WRITE SAME (16)
"""

And there were some problems with this change (specifically QEMU SCSI
WRITE SAME implementation). So the change (commit e461338b6cd4) was
made to guarantee that if LBPRZ=0, after VPD 0xB2, UNMAP is still
picked. WRITESAME(16) is picked only if LBPRZ=1. This last commit
violated spec in favor of a WRITE SAME "optout" approach for QEMU.

I wonder if this should be taken to previous versions ...

-Rafael Tinoco

On Tue, Apr 26, 2016 at 8:02 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> 3.16.35-rc1 review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: "Martin K. Petersen" <martin.petersen@oracle.com>
>
> commit 7985090aa0201fa7760583f9f8e6ba41a8d4c392 upstream.
>
> The T10 SBC UNMAP command does not provide any hard guarantees that
> blocks will return zeroes on a subsequent READ. This is due to the fact
> that the device server is free to silently ignore all or parts of the
> request.
>
> The only way to ensure that a block consistently returns zeroes after
> being unmapped is to use WRITE SAME with the UNMAP bit set. Should the
> device be unable to unmap one or more blocks described by the command it
> is required to manually write zeroes to them.
>
> Until now we have preferred UNMAP over the WRITE SAME variants to
> accommodate thinly provisioned devices that predated the final SBC-3
> spec. This patch changes the heuristic so that we favor WRITE SAME(16)
> or (10) over UNMAP if these commands are marked as supported in the
> Logical Block Provisioning VPD page.
>
> The patch also disables discard_zeroes_data for devices operating in
> UNMAP mode.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/scsi/sd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -627,7 +627,7 @@ static void sd_config_discard(struct scs
>         unsigned int logical_block_size = sdkp->device->sector_size;
>         unsigned int max_blocks = 0;
>
> -       q->limits.discard_zeroes_data = sdkp->lbprz;
> +       q->limits.discard_zeroes_data = 0;
>         q->limits.discard_alignment = sdkp->unmap_alignment *
>                 logical_block_size;
>         q->limits.discard_granularity =
> @@ -651,11 +651,13 @@ static void sd_config_discard(struct scs
>         case SD_LBP_WS16:
>                 max_blocks = min_not_zero(sdkp->max_ws_blocks,
>                                           (u32)SD_MAX_WS16_BLOCKS);
> +               q->limits.discard_zeroes_data = sdkp->lbprz;
>                 break;
>
>         case SD_LBP_WS10:
>                 max_blocks = min_not_zero(sdkp->max_ws_blocks,
>                                           (u32)SD_MAX_WS10_BLOCKS);
> +               q->limits.discard_zeroes_data = sdkp->lbprz;
>                 break;
>
>         case SD_LBP_ZERO:
> @@ -2572,12 +2574,12 @@ static void sd_read_block_limits(struct
>
>                 } else {        /* LBP VPD page tells us what to use */
>
> -                       if (sdkp->lbpu && sdkp->max_unmap_blocks)
> -                               sd_config_discard(sdkp, SD_LBP_UNMAP);
> -                       else if (sdkp->lbpws)
> +                       if (sdkp->lbpws)
>                                 sd_config_discard(sdkp, SD_LBP_WS16);
>                         else if (sdkp->lbpws10)
>                                 sd_config_discard(sdkp, SD_LBP_WS10);
> +                       else if (sdkp->lbpu && sdkp->max_unmap_blocks)
> +                               sd_config_discard(sdkp, SD_LBP_UNMAP);
>                         else
>                                 sd_config_discard(sdkp, SD_LBP_DISABLE);
>                 }
>



-- 
Rafael David Tinoco
Canonical - Kernel & Userland Sustaining Engineer
Server Tech Lead for SEG - Manager: Brooks Warner
-
# Email: rafael.tinoco@canonical.com (GPG: 2B15B499)
# LP: ~inaddy | IRC: tinoco

[toc] | [prev] | [next] | [standalone]

#1389705

>>>>> "Rafael" == Rafael David Tinoco <rafael.tinoco@canonical.com> writes:

Rafael> It seems that changing discard method from UNMAP to WRITE
Rafael> SAME(16) without using NDOB bit (as first described in
Rafael> sbc3r35b.pdf) can cause performance problems on big discards
Rafael> (since data-out buffer will be checked for every WRITE SAME
Rafael> command). I think this is happening after this commit, since
Rafael> NDOB bit wasn't implemented with this change (afaik, iirc).

We don't currently use NDOB since there is no VPD parameter to key off
of. I am also not aware of any devices that actually support it.

I am working on some patches that changes how we submit write same and
unmap down the stack to make it easier for the target subsystem to
provide zeroing guarantees. Those changes will clean some of this up but
they are 4.8 material.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1390367

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-04-27 at 17:43 -0300, Rafael David Tinoco wrote:
> It seems that changing discard method from UNMAP to WRITE SAME(16)
> without using NDOB bit (as first described in sbc3r35b.pdf) can cause
> performance problems on big discards (since data-out buffer will be
> checked for every WRITE SAME command). I think this is happening after
> this commit, since NDOB bit wasn't implemented with this change
> (afaik, iirc).

Is that an objection, or just a comment?

I only picked this commit for backporting because it was referenced by
later fixes (commits 397737223c59, f4327a95dd08) and I read the commit
message as saying that it fixes data corruption (sd claims to be
writing zeroes but the whole area might not read back as zeroes).  Is
my understanding correct?

Ben.

> From the spec:
> """
> To ensure that subsequent read operations return all zeros in a
> logical block, use the WRITE SAME (16)
> command with the NDOB bit set to one. If the UNMAP bit is set to one,
> then the device server may unmap the logical blocks specified by the
> WRITE SAME (16)
> """
> 
> And there were some problems with this change (specifically QEMU SCSI
> WRITE SAME implementation). So the change (commit e461338b6cd4) was
> made to guarantee that if LBPRZ=0, after VPD 0xB2, UNMAP is still
> picked. WRITESAME(16) is picked only if LBPRZ=1. This last commit
> violated spec in favor of a WRITE SAME "optout" approach for QEMU.
> 
> I wonder if this should be taken to previous versions ...
> 
> -Rafael Tinoco
> 
> On Tue, Apr 26, 2016 at 8:02 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > 
> > 3.16.35-rc1 review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: "Martin K. Petersen" <martin.petersen@oracle.com>
> > 
> > commit 7985090aa0201fa7760583f9f8e6ba41a8d4c392 upstream.
> > 
> > The T10 SBC UNMAP command does not provide any hard guarantees that
> > blocks will return zeroes on a subsequent READ. This is due to the fact
> > that the device server is free to silently ignore all or parts of the
> > request.
> > 
> > The only way to ensure that a block consistently returns zeroes after
> > being unmapped is to use WRITE SAME with the UNMAP bit set. Should the
> > device be unable to unmap one or more blocks described by the command it
> > is required to manually write zeroes to them.
> > 
> > Until now we have preferred UNMAP over the WRITE SAME variants to
> > accommodate thinly provisioned devices that predated the final SBC-3
> > spec. This patch changes the heuristic so that we favor WRITE SAME(16)
> > or (10) over UNMAP if these commands are marked as supported in the
> > Logical Block Provisioning VPD page.
> > 
> > The patch also disables discard_zeroes_data for devices operating in
> > UNMAP mode.
> > 
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> >  drivers/scsi/sd.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -627,7 +627,7 @@ static void sd_config_discard(struct scs
> >         unsigned int logical_block_size = sdkp->device->sector_size;
> >         unsigned int max_blocks = 0;
> > 
> > -       q->limits.discard_zeroes_data = sdkp->lbprz;
> > +       q->limits.discard_zeroes_data = 0;
> >         q->limits.discard_alignment = sdkp->unmap_alignment *
> >                 logical_block_size;
> >         q->limits.discard_granularity =
> > @@ -651,11 +651,13 @@ static void sd_config_discard(struct scs
> >         case SD_LBP_WS16:
> >                 max_blocks = min_not_zero(sdkp->max_ws_blocks,
> >                                           (u32)SD_MAX_WS16_BLOCKS);
> > +               q->limits.discard_zeroes_data = sdkp->lbprz;
> >                 break;
> > 
> >         case SD_LBP_WS10:
> >                 max_blocks = min_not_zero(sdkp->max_ws_blocks,
> >                                           (u32)SD_MAX_WS10_BLOCKS);
> > +               q->limits.discard_zeroes_data = sdkp->lbprz;
> >                 break;
> > 
> >         case SD_LBP_ZERO:
> > @@ -2572,12 +2574,12 @@ static void sd_read_block_limits(struct
> > 
> >                 } else {        /* LBP VPD page tells us what to use */
> > 
> > -                       if (sdkp->lbpu && sdkp->max_unmap_blocks)
> > -                               sd_config_discard(sdkp, SD_LBP_UNMAP);
> > -                       else if (sdkp->lbpws)
> > +                       if (sdkp->lbpws)
> >                                 sd_config_discard(sdkp, SD_LBP_WS16);
> >                         else if (sdkp->lbpws10)
> >                                 sd_config_discard(sdkp, SD_LBP_WS10);
> > +                       else if (sdkp->lbpu && sdkp->max_unmap_blocks)
> > +                               sd_config_discard(sdkp, SD_LBP_UNMAP);
> >                         else
> >                                 sd_config_discard(sdkp, SD_LBP_DISABLE);
> >                 }
> > 
> 
> 
-- 
Ben Hutchings
All extremists should be taken out and shot.

[toc] | [prev] | [next] | [standalone]

#1390720

Actually, It was an objection.

Knowing that WRITESAME(16), used as the discard mechanism, can cause
storage servers to misbehave (like QEMU's SCSI WRITESAME
implementation, workaround-ed by commit e461338b6cd4) and those
storage servers can't  rely on LBPRZ flag to opt out from WRITESAME as
discard mechanism (like QEMU does) since it is out of spec...

I have also seen storage servers miss-behaving with this specific
change (when changing from kernel 3.13 to 3.19, for example):

[21354.827291] Write same(16): 93 08 00 00 00 00 00 00 80 00 00 40 00 00 00 00
...
[21420.471648] sd 0:0:2:1: [sdw] FAILED Result: hostbyte=DID_OK
driverbyte=DRIVER_SENSE
[21420.471665] sd 0:0:2:1: [sdw] Sense Key : Illegal Request [current]
[21420.471670] sd 0:0:2:1: [sdw] Add. Sense: Invalid field in cdb

And this happened because the storage in question didn't set properly
"max_ws_blocks" (it was 0) from VPD 0xb0 (max_ws_blocks is calculated
using it).

Anyway, 2 examples of disk servers that had problems after this
change. IMHO the change is good for regular kernel development, and it
does guarantee further READ CMDs to read zeros from LBAs, but, it
jeopardises already functioning storage servers.

If that argument isn't enough,

Without properly setting NDOB bit in WRITESAME(16), the data buffer
will be read on every SCSI WRITESAME(16) command and that will impact
the "discard method" performance (will probably be slower than regular
UNMAP command).

So far, I'm seeing 2 motives why it shouldn't be on older kernels.

On Thu, Apr 28, 2016 at 1:11 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2016-04-27 at 17:43 -0300, Rafael David Tinoco wrote:
>> It seems that changing discard method from UNMAP to WRITE SAME(16)
>> without using NDOB bit (as first described in sbc3r35b.pdf) can cause
>> performance problems on big discards (since data-out buffer will be
>> checked for every WRITE SAME command). I think this is happening after
>> this commit, since NDOB bit wasn't implemented with this change
>> (afaik, iirc).
>
> Is that an objection, or just a comment?
>
> I only picked this commit for backporting because it was referenced by
> later fixes (commits 397737223c59, f4327a95dd08) and I read the commit
> message as saying that it fixes data corruption (sd claims to be
> writing zeroes but the whole area might not read back as zeroes).  Is
> my understanding correct?
>
> Ben.
>
>> From the spec:
>> """
>> To ensure that subsequent read operations return all zeros in a
>> logical block, use the WRITE SAME (16)
>> command with the NDOB bit set to one. If the UNMAP bit is set to one,
>> then the device server may unmap the logical blocks specified by the
>> WRITE SAME (16)
>> """
>>
>> And there were some problems with this change (specifically QEMU SCSI
>> WRITE SAME implementation). So the change (commit e461338b6cd4) was
>> made to guarantee that if LBPRZ=0, after VPD 0xB2, UNMAP is still
>> picked. WRITESAME(16) is picked only if LBPRZ=1. This last commit
>> violated spec in favor of a WRITE SAME "optout" approach for QEMU.
>>
>> I wonder if this should be taken to previous versions ...
>>
>> -Rafael Tinoco

[toc] | [prev] | [next] | [standalone]

#1391047

>>>>> "Rafael" == Rafael David Tinoco <rafael.tinoco@canonical.com> writes:

Rafael,

Rafael> And this happened because the storage in question didn't set
Rafael> properly "max_ws_blocks" (it was 0) from VPD 0xb0 (max_ws_blocks
Rafael> is calculated using it).

We appear to be talking about a device that has an internal limit but
does not advertise it in the dedicated field.

Please send me the output of:

       sg_inq
       sg_readcap -l
       sg_vpd -p lbpv
       sg_vpd -p bl

and we'll quirk it. Or feel free to submit a patch.

Rafael> Anyway, 2 examples of disk servers that had problems after this
Rafael> change. IMHO the change is good for regular kernel development,
Rafael> and it does guarantee further READ CMDs to read zeros from LBAs,
Rafael> but, it jeopardises already functioning storage servers.

... as opposed to jeopardizing the integrity of people's data?

Rafael> Without properly setting NDOB bit in WRITESAME(16), the data
Rafael> buffer will be read on every SCSI WRITESAME(16) command and that
Rafael> will impact the "discard method" performance (will probably be
Rafael> slower than regular UNMAP command).

NDOB is a recent performance optimization for high performance SSD
devices that allows us to skip sending the zeroed data buffer. However,
there is no way to tell whether a device supports NDOB without sending a
WRITE SAME command which has the nasty side effect of being destructive.
I am also not aware of any devices that actually support it yet.

Whether to set NDOB or not is completely orthogonal to the issue at hand
which is whether to use the UNMAP command or WRITE SAME with the UNMAP
bit set. WRITE SAME w/ UNMAP has been around for a long, long
time. WRITE SAME was used for thin provisioned devices before the UNMAP
command even existed.

Currently the filesystem code relies on being able to get predictable
results for zeroing metadata blocks etc. So if a device advertises that
it supports LBPRZ we'll use WRITE SAME with the UNMAP bit set since it
provides hard guarantees. Unlike the UNMAP command which by definition
is advisory.

As I mentioned earlier, I have some changes in the pipeline that will
separate the ioctl implementation from the library functions. That will
allow us to use WRITE SAME w/ UNMAP for zeroouts and UNMAP for discards
on the same device. However, this is still not going to solve the
problem with your device that fails WRITE SAME w/ UNMAP. Because we are
not going to discontinue using that combination. Quite the contrary, we
are increasingly depending on it.

Consequently, if you have a device that has problems in that area we
need to quirk it rather than try to work around it in the core code
heuristics.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1391244

Martin,

On Fri, Apr 29, 2016 at 9:16 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Rafael" == Rafael David Tinoco <rafael.tinoco@canonical.com> writes:
>
> Rafael,
>
> Rafael> And this happened because the storage in question didn't set
> Rafael> properly "max_ws_blocks" (it was 0) from VPD 0xb0 (max_ws_blocks
> Rafael> is calculated using it).
>
> We appear to be talking about a device that has an internal limit but
> does not advertise it in the dedicated field.

Exactly, Just quoted it as example of things that can brake.

For this case, fixing VPD's max_ws_blocks fixed the problem because of:

* IF max_ws_blocks > 0xffff
       max_ws_blocks = min (max_ws_blocks, 0x7fffff)

No need for a patch.

> NDOB is a recent performance optimization for high performance SSD
> devices that allows us to skip sending the zeroed data buffer. However,
> there is no way to tell whether a device supports NDOB without sending a
> WRITE SAME command which has the nasty side effect of being destructive.
> I am also not aware of any devices that actually support it yet.

Yep, no way to inquiry it, unfortunately. I'll let the vendor that
approached me to speak for themselves about their NDOB support, if
they're willing to. They faced performance issues with WRITESAME(),
but,  there is no free lunch, i'm afraid, to guarantee subsequent
READs are good.

> As I mentioned earlier, I have some changes in the pipeline that will
> separate the ioctl implementation from the library functions. That will
> allow us to use WRITE SAME w/ UNMAP for zeroouts and UNMAP for discards
> on the same device. However, this is still not going to solve the
> problem with your device that fails WRITE SAME w/ UNMAP. Because we are
> not going to discontinue using that combination. Quite the contrary, we
> are increasingly depending on it.

Definitely.

> Consequently, if you have a device that has problems in that area we
> need to quirk it rather than try to work around it in the core code
> heuristics.

Sure, my objection was because I could see 2 "storage servers"
implementation problems that appeared with this change, but I do see
your point on data guarantees.

Thank you very much for clarifying this.

--
Rafael Tinoco
Canonical Sustaining Engineering

[toc] | [prev] | [next] | [standalone]

#1391661

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-04-29 at 01:00 -0300, Rafael David Tinoco wrote:
> Actually, It was an objection.
[...]

OK, then I'll drop all these commits from the queue:

7985090aa020 sd: disable discard_zeroes_data for UNMAP
397737223c59 sd: Make discard granularity match logical block size when LBPRZ=1
f4327a95dd08 sd: Fix discard granularity when LBPRZ=1

But if sd in 3.16 is wrongly promising that discard zeroes data, it
really does need to be fixed somehow.

Ben.

-- 
Ben Hutchings
Tomorrow will be cancelled due to lack of interest.

[toc] | [prev] | [next] | [standalone]

#1392907

>>>>> "Ben" == Ben Hutchings <ben@decadent.org.uk> writes:

Ben> But if sd in 3.16 is wrongly promising that discard zeroes data, it
Ben> really does need to be fixed somehow.

If there's a regression on a particular device we should address that
rather than dropping valid fixes from core SCSI code.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1397087

On 29/04/2016 06:00, Rafael David Tinoco wrote:
> Actually, It was an objection.
> 
> Knowing that WRITESAME(16), used as the discard mechanism, can cause
> storage servers to misbehave (like QEMU's SCSI WRITESAME
> implementation, workaround-ed by commit e461338b6cd4) and those
> storage servers can't  rely on LBPRZ flag to opt out from WRITESAME as
> discard mechanism (like QEMU does) since it is out of spec...

I think e461338b6cd4 is a perfectly fine change---independent of whether
it's a workaround for QEMU---and should be backported to stable kernels too.

Is there a reason to use WRITE SAME if LBPRZ=0?  You risk doing a real
write which breaks thin provisioning and will probably take a huge time too.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web