Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "Christoph" == hch@infradead org writes: Christoph> That's mostly because we don't support larger than 512 byte Christoph> TRIM payloads yet.. I did add support for that a few years back but all hell broke loose and we had to revert it. There were several drives that failed with more than 512 bytes of payload despite advertising support for 4K. It's the same problem that we have now with queued TRIM. There are several vendors that have implemented it but until we added support in Linux they had no way of testing it. And as a result their implementations are buggy. Even with a Linux implementation readily available it's hard to get them to test since Linux is not a tier 1 platform in the consumer segment. For enterprise drives it's an entirely different matter, of course... -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 03:20:00PM -0400, Martin K. Petersen wrote: > The block layer can only describe one contiguous block range in a > request. My copy offload patches introduces the bi_special field that > allows us to attach additional information to an I/O. I have > experimented with doing that for discards to overcome the suck of DSM > TRIM. Splitting and merging requests in MD/DM gets much more cumbersome, > though. I had a bunch of prototypes for this years ago that didn't really work out. I always made ranged didscards something that driver had to opt in for. In my case md and dm never opted in, although for mirroring or multipath it should of course handle it fairly easily. > It also wasn't evident that it was worth the hassle. While UNMAP allows > us to express large regions, DSM TRIM on the SATA side is limited to 32 > MB per range. So in many cases we end up maxing out the payload capacity > even with a single contiguous range. That's mostly because we don't support larger than 512 byte TRIM payloads yet.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 03:20:00PM -0400, Martin K. Petersen wrote: The block layer can only describe one contiguous block range in a request. My copy offload patches introduces the bi_special field that allows us to attach additional information to an I/O. I have experimented with doing that for discards to overcome the suck of DSM TRIM. Splitting and merging requests in MD/DM gets much more cumbersome, though. I had a bunch of prototypes for this years ago that didn't really work out. I always made ranged didscards something that driver had to opt in for. In my case md and dm never opted in, although for mirroring or multipath it should of course handle it fairly easily. It also wasn't evident that it was worth the hassle. While UNMAP allows us to express large regions, DSM TRIM on the SATA side is limited to 32 MB per range. So in many cases we end up maxing out the payload capacity even with a single contiguous range. That's mostly because we don't support larger than 512 byte TRIM payloads yet.. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
Christoph == hch@infradead org h...@infradead.org writes: Christoph That's mostly because we don't support larger than 512 byte Christoph TRIM payloads yet.. I did add support for that a few years back but all hell broke loose and we had to revert it. There were several drives that failed with more than 512 bytes of payload despite advertising support for 4K. It's the same problem that we have now with queued TRIM. There are several vendors that have implemented it but until we added support in Linux they had no way of testing it. And as a result their implementations are buggy. Even with a Linux implementation readily available it's hard to get them to test since Linux is not a tier 1 platform in the consumer segment. For enterprise drives it's an entirely different matter, of course... -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "James" == James Bottomley writes: James> Well, your judgement: is this situation (support for UNMAP but James> not for WRITE_SAME) in what is effectively a RAID driver (hv James> drivers count as RAID) just a silly Microsoft one off? I only recall seeing one or two devices that supported LBP but not WRITE SAME w/UNMAP. James> However, if we get any RAID drivers with strange discard support, James> we'll likely get the same problem The LBP VPD page is mandatory now. It wasn't for the first couple of generations of devices that we still have to support. I think that if a vendor were to support LBP, adding the mandatory VPD page would be a given. And so far nobody has messed up the LBP VPD page contents. My main gripe about linking no_write_same and discard functionality is that the heuristics for the latter are already excessively complex thanks to having to support devices that predate the spec. I'm wary of adding another dimension to that. Also, linking the two use cases we can get into inconsistent states where no_write_same is set but the device does not support UNMAP and has LBPWS=1 and LBPWS10=1 set in the LBP VPD. I'll contemplate the LBPME => mandatory VPD lookup thing for bit. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "Rob" == Elliott, Robert (Server Storage) writes: Rob> WRITE SAME with the UNMAP bit set to one (and a few other Rob> conditions) guarantees that the data is zeroed out, while the UNMAP Rob> command is just a hint. They're not fully interchangeable. Which Rob> semantics are implied by REQ_DISCARD and these functions? Voluntary deprovisioning of a block range. Rob> One benefit of UNMAP is that UNMAP supports a list of discontiguous Rob> LBA ranges, whereas WRITE SAME just supports one LBA range. Rob> sd_setup_discard_cmnd is not taking advantage of this feature, Rob> though. The block layer can only describe one contiguous block range in a request. My copy offload patches introduces the bi_special field that allows us to attach additional information to an I/O. I have experimented with doing that for discards to overcome the suck of DSM TRIM. Splitting and merging requests in MD/DM gets much more cumbersome, though. Rob> Ideally, the block layer would merge multiple discards into one Rob> UNMAP command if they're stuck on the request queue for a while, Rob> like it merges adjacent reads and writes. We support merging contiguous smaller discard requests. We did discuss having a (separate) I/O scheduler to coalesce discontiguous discard requests. However, the attempts at this turned out to be pretty hideous. It also wasn't evident that it was worth the hassle. While UNMAP allows us to express large regions, DSM TRIM on the SATA side is limited to 32 MB per range. So in many cases we end up maxing out the payload capacity even with a single contiguous range. We expect LBP SCSI devices to queue commands. Being able to express multiple ranges in one shot is less critical in that case. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 15:08 -0400, Martin K. Petersen wrote: > > "James" == James Bottomley writes: > > James> It's the code we identified in sd.c:read_capacity_16(): > > That's there to support devices that implement thin provisioning but > which predate the LBP VPD page. And thus have no way to tell us their > preferred command variant. > > James> If the inquiry shows LBPME set, we'll do write same regardless of > James> the no_write_same bit. That's why for SPC-2 devices it only > James> shows up on >> 2TB devices because they force RC16. > > But we only do so if they have LBPME set. Generally they don't. > > The problem for hyperv was that LBPME was set. And because it claimed > SBC-2 we did not check the LBP VPD page to see that it prefers UNMAP. > > I believe we did consider unconditionally checking for VPDs when LBPME > was set but I seem to recall that it broke some device that had garbage > in the READ CAPACITY(16) response. I'll see if I can locate the details. > > Otherwise I'm willing to entertain that idea. Well, your judgement: is this situation (support for UNMAP but not for WRITE_SAME) in what is effectively a RAID driver (hv drivers count as RAID) just a silly Microsoft one off? If so, we can go with your force VPD page patch. However, if we get any RAID drivers with strange discard support, we'll likely get the same problem ... I've got to say in the long run, it sounds likely we will given that RAID vendors are only marginally more competent than USB bridge vendors when it comes to following the spec. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "James" == James Bottomley writes: James> It's the code we identified in sd.c:read_capacity_16(): That's there to support devices that implement thin provisioning but which predate the LBP VPD page. And thus have no way to tell us their preferred command variant. James> If the inquiry shows LBPME set, we'll do write same regardless of James> the no_write_same bit. That's why for SPC-2 devices it only James> shows up on >> 2TB devices because they force RC16. But we only do so if they have LBPME set. Generally they don't. The problem for hyperv was that LBPME was set. And because it claimed SBC-2 we did not check the LBP VPD page to see that it prefers UNMAP. I believe we did consider unconditionally checking for VPDs when LBPME was set but I seem to recall that it broke some device that had garbage in the READ CAPACITY(16) response. I'll see if I can locate the details. Otherwise I'm willing to entertain that idea. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 14:45 -0400, Martin K. Petersen wrote:
> > "James" == James Bottomley writes:
>
> >> I don't have a problem with a BLIST_PREFER_UNMAP flag or something
> >> like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it
> >> does fix the problem at hand. That's why I went that route.
>
> James> Hang on ... unless we apply Christoph or my fix, we'll get the
> James> same issue with every raid driver (that's about 10 I think) that
> James> set no_write_same when they hit a >2TB RAID volume,
>
> no_write_same is set for all the RAID controller drivers (54b2b50c20a6).
>
> If a WRITE SAME(10/16) command fails and the UNMAP bit is not set we'll
> set no_write_same=1 and disable REQ_WRITE_SAME support.
>
> If a WRITE SAME(10/16) command fails and the UNMAP bit is set we'll
> disable REQ_DISCARD support.
>
> Not sure where the 10 vs. 16 byte 2TB limitation comes into play here?
It's the code we identified in sd.c:read_capacity_16():
if (buffer[14] & 0x80) { /* LBPME */
sdkp->lbpme = 1;
if (buffer[14] & 0x40) /* LBPRZ */
sdkp->lbprz = 1;
sd_config_discard(sdkp, SD_LBP_WS16);
}
If the inquiry shows LBPME set, we'll do write same regardless of the
no_write_same bit. That's why for SPC-2 devices it only shows up on
>2TB devices because they force RC16.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "James" == James Bottomley writes: >> I don't have a problem with a BLIST_PREFER_UNMAP flag or something >> like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it >> does fix the problem at hand. That's why I went that route. James> Hang on ... unless we apply Christoph or my fix, we'll get the James> same issue with every raid driver (that's about 10 I think) that James> set no_write_same when they hit a >2TB RAID volume, no_write_same is set for all the RAID controller drivers (54b2b50c20a6). If a WRITE SAME(10/16) command fails and the UNMAP bit is not set we'll set no_write_same=1 and disable REQ_WRITE_SAME support. If a WRITE SAME(10/16) command fails and the UNMAP bit is set we'll disable REQ_DISCARD support. Not sure where the 10 vs. 16 byte 2TB limitation comes into play here? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- > ow...@vger.kernel.org] On Behalf Of James Bottomley > Sent: Wednesday, 16 July, 2014 1:02 PM > To: martin.peter...@oracle.com > Cc: linux-kernel@vger.kernel.org; h...@infradead.org; > de...@linuxdriverproject.org; a...@canonical.com; k...@microsoft.com; > sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; > jasow...@redhat.com > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > On Wed, 2014-07-16 at 13:47 -0400, Martin K. Petersen wrote: > > >>>>> "Christoph" == hch@infradead org writes: > > > > Christoph> Oh, we actually have devices that support WRITE SAME with > > Christoph> unmap, but not without? That's defintively a little strange. > > > > Yep :( > > > > There were several SSDs that did not want to support wearing out flash > > by writing gobs of zeroes and only support the UNMAP case. > > > > Christoph> Yes, and it did this intentionally. I really wouldn't expect > > Christoph> devices to support WRITE SAME with UNMAP but blow up on a > > Christoph> WRITE SAME without it (and not just simple fail it in an > > Christoph> orderly way). > > > > *sigh* > > > > Christoph> It definitively seems odd to default to trying WRITE SAME for > > Christoph> unmap for a device that explicitly tells us that it doesn't > > Christoph> support WRITE SAME. > > > > Maybe it's just a naming thing. I was really trying to convey > > no_req_write_same support, not no_write_same_10_or_16. > > > > Christoph> Note that I'm not against your patch - I suspect forcing us > > Christoph> to read EVPD pages even for devices that claim to be SPC-2 > > Christoph> will come in useful in various scenarios. > > > > I don't have a problem with a BLIST_PREFER_UNMAP flag or something like > > that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does > > fix the problem at hand. That's why I went that route. > > Hang on ... unless we apply Christoph or my fix, we'll get the same > issue with every raid driver (that's about 10 I think) that set > no_write_same when they hit a >2TB RAID volume, so I think we need both > fixes. > > James > WRITE SAME with the UNMAP bit set to one (and a few other conditions) guarantees that the data is zeroed out, while the UNMAP command is just a hint. They're not fully interchangeable. Which semantics are implied by REQ_DISCARD and these functions? One benefit of UNMAP is that UNMAP supports a list of discontiguous LBA ranges, whereas WRITE SAME just supports one LBA range. sd_setup_discard_cmnd is not taking advantage of this feature, though. Ideally, the block layer would merge multiple discards into one UNMAP command if they're stuck on the request queue for a while, like it merges adjacent reads and writes. That would pave the way for building up WRITE SCATTERED and READ GATHERED commands. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 13:47 -0400, Martin K. Petersen wrote: > > "Christoph" == hch@infradead org writes: > > Christoph> Oh, we actually have devices that support WRITE SAME with > Christoph> unmap, but not without? That's defintively a little strange. > > Yep :( > > There were several SSDs that did not want to support wearing out flash > by writing gobs of zeroes and only support the UNMAP case. > > Christoph> Yes, and it did this intentionally. I really wouldn't expect > Christoph> devices to support WRITE SAME with UNMAP but blow up on a > Christoph> WRITE SAME without it (and not just simple fail it in an > Christoph> orderly way). > > *sigh* > > Christoph> It definitively seems odd to default to trying WRITE SAME for > Christoph> unmap for a device that explicitly tells us that it doesn't > Christoph> support WRITE SAME. > > Maybe it's just a naming thing. I was really trying to convey > no_req_write_same support, not no_write_same_10_or_16. > > Christoph> Note that I'm not against your patch - I suspect forcing us > Christoph> to read EVPD pages even for devices that claim to be SPC-2 > Christoph> will come in useful in various scenarios. > > I don't have a problem with a BLIST_PREFER_UNMAP flag or something like > that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does > fix the problem at hand. That's why I went that route. Hang on ... unless we apply Christoph or my fix, we'll get the same issue with every raid driver (that's about 10 I think) that set no_write_same when they hit a >2TB RAID volume, so I think we need both fixes. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 01:47:35PM -0400, Martin K. Petersen wrote: > There were several SSDs that did not want to support wearing out flash > by writing gobs of zeroes and only support the UNMAP case. Given that SSDs usually aren't hard provisioned anyway that seems like an odd decision. But SAS SSD would be something I'd at least expect to properly fail these.. > I don't have a problem with a BLIST_PREFER_UNMAP flag or something like > that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does > fix the problem at hand. That's why I went that route. As I said I'm perfectly fine with your patch and I think we'll find more uses for it. I'll apply it as soon as I get a second review. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "Christoph" == hch@infradead org writes: Christoph> Oh, we actually have devices that support WRITE SAME with Christoph> unmap, but not without? That's defintively a little strange. Yep :( There were several SSDs that did not want to support wearing out flash by writing gobs of zeroes and only support the UNMAP case. Christoph> Yes, and it did this intentionally. I really wouldn't expect Christoph> devices to support WRITE SAME with UNMAP but blow up on a Christoph> WRITE SAME without it (and not just simple fail it in an Christoph> orderly way). *sigh* Christoph> It definitively seems odd to default to trying WRITE SAME for Christoph> unmap for a device that explicitly tells us that it doesn't Christoph> support WRITE SAME. Maybe it's just a naming thing. I was really trying to convey no_req_write_same support, not no_write_same_10_or_16. Christoph> Note that I'm not against your patch - I suspect forcing us Christoph> to read EVPD pages even for devices that claim to be SPC-2 Christoph> will come in useful in various scenarios. I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 11:44:18AM -0400, Martin K. Petersen wrote: > There are lots of devices out there that support WRITE SAME(10) or (16) > without the UNMAP bit. And there are devices that support WRITE SAME w/ > UNMAP functionality but not "regular" WRITE SAME. Oh, we actually have devices that support WRITE SAME with unmap, but not without? That's defintively a little strange. > no_write_same is there to prevent the REQ_WRITE_SAME use case (for which > we have really weak heuristics). Your patch overloads no_write_same so > it also governs a REQ_DISCARD use case. Yes, and it did this intentionally. I really wouldn't expect devices to support WRITE SAME with UNMAP but blow up on a WRITE SAME without it (and not just simple fail it in an orderly way). > My proposed black list patch fixes the hyperv discard issue. So I don't > see why we'd need to overload no_write_same which was meant for an > entirely different purpose. It definitively seems odd to default to trying WRITE SAME for unmap for a device that explicitly tells us that it doesn't support WRITE SAME. Note that I'm not against your patch - I suspect forcing us to read EVPD pages even for devices that claim to be SPC-2 will come in useful in various scenarios. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "hch" == hch@infradead org writes: hch> read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the hch> LPBME bit is set. At least older SBC drafts left it wide open if a hch> target supports WRITE SAME with UNMAP or UNMAP in this case. Correct. hch> So I think we'd still want a patch to use UNMAP instead of WRITE hch> SAME for this case, which should also fix hyperv. There are lots of devices out there that support WRITE SAME(10) or (16) without the UNMAP bit. And there are devices that support WRITE SAME w/ UNMAP functionality but not "regular" WRITE SAME. no_write_same is there to prevent the REQ_WRITE_SAME use case (for which we have really weak heuristics). Your patch overloads no_write_same so it also governs a REQ_DISCARD use case. My proposed black list patch fixes the hyperv discard issue. So I don't see why we'd need to overload no_write_same which was meant for an entirely different purpose. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 04:01 -0700, h...@infradead.org wrote: > On Sun, Jul 13, 2014 at 08:58:34AM -0400, Martin K. Petersen wrote: > > > "KY" == KY Srinivasan writes: > > > > KY> Windows hosts do support UNMAP and set the field in the > > KY> EVPD. However, since the host advertises SPC-2 compliance, Linux > > KY> does not even query the VPD page. > > > > >> If we want to enable UNMAP in this case I'd prefer a blacklist entry > > >> than trying UNMAP despite the device not advertising it. > > > > I agree with that. We could do something like the patch below. > > > > However, I do think it's a good idea that you guys are looking into > > reporting SPC-3. > > KY mentioned that they have a prototype for that now. > > Btw, I looked over sd.c a bit more, and I think I understand why they > get the WRITE SAME commands now: > > read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the LPBME > bit is set. At least older SBC drafts left it wide open if a target > supports WRITE SAME with UNMAP or UNMAP in this case. So I think we'd > still want a patch to use UNMAP instead of WRITE SAME for this case, > which should also fix hyperv. Below is the quick hack version of that > that just checks the host no_write_same flag, as the one on the device > isn't set yet - I guess we need to refactor some of that logic. > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 87566b5..4480fdf 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2035,7 +2035,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, > struct scsi_device *sdp, > if (buffer[14] & 0x40) /* LBPRZ */ > sdkp->lbprz = 1; > > - sd_config_discard(sdkp, SD_LBP_WS16); > + if (sdp->host->no_write_same) > + sd_config_discard(sdkp, SD_LBP_UNMAP); > + else > + sd_config_discard(sdkp, SD_LBP_WS16); Right, I already said this was the problem: that was the reason for my patch. However, there are a couple of other cases (including the /sys entry) which is why I patched sd_config_discard instead. James
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Sun, Jul 13, 2014 at 08:58:34AM -0400, Martin K. Petersen wrote: > > "KY" == KY Srinivasan writes: > > KY> Windows hosts do support UNMAP and set the field in the > KY> EVPD. However, since the host advertises SPC-2 compliance, Linux > KY> does not even query the VPD page. > > >> If we want to enable UNMAP in this case I'd prefer a blacklist entry > >> than trying UNMAP despite the device not advertising it. > > I agree with that. We could do something like the patch below. > > However, I do think it's a good idea that you guys are looking into > reporting SPC-3. KY mentioned that they have a prototype for that now. Btw, I looked over sd.c a bit more, and I think I understand why they get the WRITE SAME commands now: read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the LPBME bit is set. At least older SBC drafts left it wide open if a target supports WRITE SAME with UNMAP or UNMAP in this case. So I think we'd still want a patch to use UNMAP instead of WRITE SAME for this case, which should also fix hyperv. Below is the quick hack version of that that just checks the host no_write_same flag, as the one on the device isn't set yet - I guess we need to refactor some of that logic. diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 87566b5..4480fdf 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2035,7 +2035,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp, if (buffer[14] & 0x40) /* LBPRZ */ sdkp->lbprz = 1; - sd_config_discard(sdkp, SD_LBP_WS16); + if (sdp->host->no_write_same) + sd_config_discard(sdkp, SD_LBP_UNMAP); + else + sd_config_discard(sdkp, SD_LBP_WS16); } sdkp->capacity = lba + 1; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Sun, Jul 13, 2014 at 08:58:34AM -0400, Martin K. Petersen wrote: KY == KY Srinivasan k...@microsoft.com writes: KY Windows hosts do support UNMAP and set the field in the KY EVPD. However, since the host advertises SPC-2 compliance, Linux KY does not even query the VPD page. If we want to enable UNMAP in this case I'd prefer a blacklist entry than trying UNMAP despite the device not advertising it. I agree with that. We could do something like the patch below. However, I do think it's a good idea that you guys are looking into reporting SPC-3. KY mentioned that they have a prototype for that now. Btw, I looked over sd.c a bit more, and I think I understand why they get the WRITE SAME commands now: read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the LPBME bit is set. At least older SBC drafts left it wide open if a target supports WRITE SAME with UNMAP or UNMAP in this case. So I think we'd still want a patch to use UNMAP instead of WRITE SAME for this case, which should also fix hyperv. Below is the quick hack version of that that just checks the host no_write_same flag, as the one on the device isn't set yet - I guess we need to refactor some of that logic. diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 87566b5..4480fdf 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2035,7 +2035,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp, if (buffer[14] 0x40) /* LBPRZ */ sdkp-lbprz = 1; - sd_config_discard(sdkp, SD_LBP_WS16); + if (sdp-host-no_write_same) + sd_config_discard(sdkp, SD_LBP_UNMAP); + else + sd_config_discard(sdkp, SD_LBP_WS16); } sdkp-capacity = lba + 1; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 04:01 -0700, h...@infradead.org wrote: On Sun, Jul 13, 2014 at 08:58:34AM -0400, Martin K. Petersen wrote: KY == KY Srinivasan k...@microsoft.com writes: KY Windows hosts do support UNMAP and set the field in the KY EVPD. However, since the host advertises SPC-2 compliance, Linux KY does not even query the VPD page. If we want to enable UNMAP in this case I'd prefer a blacklist entry than trying UNMAP despite the device not advertising it. I agree with that. We could do something like the patch below. However, I do think it's a good idea that you guys are looking into reporting SPC-3. KY mentioned that they have a prototype for that now. Btw, I looked over sd.c a bit more, and I think I understand why they get the WRITE SAME commands now: read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the LPBME bit is set. At least older SBC drafts left it wide open if a target supports WRITE SAME with UNMAP or UNMAP in this case. So I think we'd still want a patch to use UNMAP instead of WRITE SAME for this case, which should also fix hyperv. Below is the quick hack version of that that just checks the host no_write_same flag, as the one on the device isn't set yet - I guess we need to refactor some of that logic. diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 87566b5..4480fdf 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2035,7 +2035,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp, if (buffer[14] 0x40) /* LBPRZ */ sdkp-lbprz = 1; - sd_config_discard(sdkp, SD_LBP_WS16); + if (sdp-host-no_write_same) + sd_config_discard(sdkp, SD_LBP_UNMAP); + else + sd_config_discard(sdkp, SD_LBP_WS16); Right, I already said this was the problem: that was the reason for my patch. However, there are a couple of other cases (including the /sys entry) which is why I patched sd_config_discard instead. James
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
hch == hch@infradead org h...@infradead.org writes: hch read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the hch LPBME bit is set. At least older SBC drafts left it wide open if a hch target supports WRITE SAME with UNMAP or UNMAP in this case. Correct. hch So I think we'd still want a patch to use UNMAP instead of WRITE hch SAME for this case, which should also fix hyperv. There are lots of devices out there that support WRITE SAME(10) or (16) without the UNMAP bit. And there are devices that support WRITE SAME w/ UNMAP functionality but not regular WRITE SAME. no_write_same is there to prevent the REQ_WRITE_SAME use case (for which we have really weak heuristics). Your patch overloads no_write_same so it also governs a REQ_DISCARD use case. My proposed black list patch fixes the hyperv discard issue. So I don't see why we'd need to overload no_write_same which was meant for an entirely different purpose. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 11:44:18AM -0400, Martin K. Petersen wrote: There are lots of devices out there that support WRITE SAME(10) or (16) without the UNMAP bit. And there are devices that support WRITE SAME w/ UNMAP functionality but not regular WRITE SAME. Oh, we actually have devices that support WRITE SAME with unmap, but not without? That's defintively a little strange. no_write_same is there to prevent the REQ_WRITE_SAME use case (for which we have really weak heuristics). Your patch overloads no_write_same so it also governs a REQ_DISCARD use case. Yes, and it did this intentionally. I really wouldn't expect devices to support WRITE SAME with UNMAP but blow up on a WRITE SAME without it (and not just simple fail it in an orderly way). My proposed black list patch fixes the hyperv discard issue. So I don't see why we'd need to overload no_write_same which was meant for an entirely different purpose. It definitively seems odd to default to trying WRITE SAME for unmap for a device that explicitly tells us that it doesn't support WRITE SAME. Note that I'm not against your patch - I suspect forcing us to read EVPD pages even for devices that claim to be SPC-2 will come in useful in various scenarios. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
Christoph == hch@infradead org h...@infradead.org writes: Christoph Oh, we actually have devices that support WRITE SAME with Christoph unmap, but not without? That's defintively a little strange. Yep :( There were several SSDs that did not want to support wearing out flash by writing gobs of zeroes and only support the UNMAP case. Christoph Yes, and it did this intentionally. I really wouldn't expect Christoph devices to support WRITE SAME with UNMAP but blow up on a Christoph WRITE SAME without it (and not just simple fail it in an Christoph orderly way). *sigh* Christoph It definitively seems odd to default to trying WRITE SAME for Christoph unmap for a device that explicitly tells us that it doesn't Christoph support WRITE SAME. Maybe it's just a naming thing. I was really trying to convey no_req_write_same support, not no_write_same_10_or_16. Christoph Note that I'm not against your patch - I suspect forcing us Christoph to read EVPD pages even for devices that claim to be SPC-2 Christoph will come in useful in various scenarios. I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 16, 2014 at 01:47:35PM -0400, Martin K. Petersen wrote: There were several SSDs that did not want to support wearing out flash by writing gobs of zeroes and only support the UNMAP case. Given that SSDs usually aren't hard provisioned anyway that seems like an odd decision. But SAS SSD would be something I'd at least expect to properly fail these.. I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. As I said I'm perfectly fine with your patch and I think we'll find more uses for it. I'll apply it as soon as I get a second review. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 13:47 -0400, Martin K. Petersen wrote: Christoph == hch@infradead org h...@infradead.org writes: Christoph Oh, we actually have devices that support WRITE SAME with Christoph unmap, but not without? That's defintively a little strange. Yep :( There were several SSDs that did not want to support wearing out flash by writing gobs of zeroes and only support the UNMAP case. Christoph Yes, and it did this intentionally. I really wouldn't expect Christoph devices to support WRITE SAME with UNMAP but blow up on a Christoph WRITE SAME without it (and not just simple fail it in an Christoph orderly way). *sigh* Christoph It definitively seems odd to default to trying WRITE SAME for Christoph unmap for a device that explicitly tells us that it doesn't Christoph support WRITE SAME. Maybe it's just a naming thing. I was really trying to convey no_req_write_same support, not no_write_same_10_or_16. Christoph Note that I'm not against your patch - I suspect forcing us Christoph to read EVPD pages even for devices that claim to be SPC-2 Christoph will come in useful in various scenarios. I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. Hang on ... unless we apply Christoph or my fix, we'll get the same issue with every raid driver (that's about 10 I think) that set no_write_same when they hit a 2TB RAID volume, so I think we need both fixes. James -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message- From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- ow...@vger.kernel.org] On Behalf Of James Bottomley Sent: Wednesday, 16 July, 2014 1:02 PM To: martin.peter...@oracle.com Cc: linux-kernel@vger.kernel.org; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; k...@microsoft.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-16 at 13:47 -0400, Martin K. Petersen wrote: Christoph == hch@infradead org h...@infradead.org writes: Christoph Oh, we actually have devices that support WRITE SAME with Christoph unmap, but not without? That's defintively a little strange. Yep :( There were several SSDs that did not want to support wearing out flash by writing gobs of zeroes and only support the UNMAP case. Christoph Yes, and it did this intentionally. I really wouldn't expect Christoph devices to support WRITE SAME with UNMAP but blow up on a Christoph WRITE SAME without it (and not just simple fail it in an Christoph orderly way). *sigh* Christoph It definitively seems odd to default to trying WRITE SAME for Christoph unmap for a device that explicitly tells us that it doesn't Christoph support WRITE SAME. Maybe it's just a naming thing. I was really trying to convey no_req_write_same support, not no_write_same_10_or_16. Christoph Note that I'm not against your patch - I suspect forcing us Christoph to read EVPD pages even for devices that claim to be SPC-2 Christoph will come in useful in various scenarios. I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. Hang on ... unless we apply Christoph or my fix, we'll get the same issue with every raid driver (that's about 10 I think) that set no_write_same when they hit a 2TB RAID volume, so I think we need both fixes. James WRITE SAME with the UNMAP bit set to one (and a few other conditions) guarantees that the data is zeroed out, while the UNMAP command is just a hint. They're not fully interchangeable. Which semantics are implied by REQ_DISCARD and these functions? One benefit of UNMAP is that UNMAP supports a list of discontiguous LBA ranges, whereas WRITE SAME just supports one LBA range. sd_setup_discard_cmnd is not taking advantage of this feature, though. Ideally, the block layer would merge multiple discards into one UNMAP command if they're stuck on the request queue for a while, like it merges adjacent reads and writes. That would pave the way for building up WRITE SCATTERED and READ GATHERED commands. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
James == James Bottomley jbottom...@parallels.com writes: I don't have a problem with a BLIST_PREFER_UNMAP flag or something like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does fix the problem at hand. That's why I went that route. James Hang on ... unless we apply Christoph or my fix, we'll get the James same issue with every raid driver (that's about 10 I think) that James set no_write_same when they hit a 2TB RAID volume, no_write_same is set for all the RAID controller drivers (54b2b50c20a6). If a WRITE SAME(10/16) command fails and the UNMAP bit is not set we'll set no_write_same=1 and disable REQ_WRITE_SAME support. If a WRITE SAME(10/16) command fails and the UNMAP bit is set we'll disable REQ_DISCARD support. Not sure where the 10 vs. 16 byte 2TB limitation comes into play here? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 14:45 -0400, Martin K. Petersen wrote:
James == James Bottomley jbottom...@parallels.com writes:
I don't have a problem with a BLIST_PREFER_UNMAP flag or something
like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it
does fix the problem at hand. That's why I went that route.
James Hang on ... unless we apply Christoph or my fix, we'll get the
James same issue with every raid driver (that's about 10 I think) that
James set no_write_same when they hit a 2TB RAID volume,
no_write_same is set for all the RAID controller drivers (54b2b50c20a6).
If a WRITE SAME(10/16) command fails and the UNMAP bit is not set we'll
set no_write_same=1 and disable REQ_WRITE_SAME support.
If a WRITE SAME(10/16) command fails and the UNMAP bit is set we'll
disable REQ_DISCARD support.
Not sure where the 10 vs. 16 byte 2TB limitation comes into play here?
It's the code we identified in sd.c:read_capacity_16():
if (buffer[14] 0x80) { /* LBPME */
sdkp-lbpme = 1;
if (buffer[14] 0x40) /* LBPRZ */
sdkp-lbprz = 1;
sd_config_discard(sdkp, SD_LBP_WS16);
}
If the inquiry shows LBPME set, we'll do write same regardless of the
no_write_same bit. That's why for SPC-2 devices it only shows up on
2TB devices because they force RC16.
James
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
James == James Bottomley jbottom...@parallels.com writes: James It's the code we identified in sd.c:read_capacity_16(): That's there to support devices that implement thin provisioning but which predate the LBP VPD page. And thus have no way to tell us their preferred command variant. James If the inquiry shows LBPME set, we'll do write same regardless of James the no_write_same bit. That's why for SPC-2 devices it only James shows up on 2TB devices because they force RC16. But we only do so if they have LBPME set. Generally they don't. The problem for hyperv was that LBPME was set. And because it claimed SBC-2 we did not check the LBP VPD page to see that it prefers UNMAP. I believe we did consider unconditionally checking for VPDs when LBPME was set but I seem to recall that it broke some device that had garbage in the READ CAPACITY(16) response. I'll see if I can locate the details. Otherwise I'm willing to entertain that idea. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 15:08 -0400, Martin K. Petersen wrote: James == James Bottomley jbottom...@parallels.com writes: James It's the code we identified in sd.c:read_capacity_16(): That's there to support devices that implement thin provisioning but which predate the LBP VPD page. And thus have no way to tell us their preferred command variant. James If the inquiry shows LBPME set, we'll do write same regardless of James the no_write_same bit. That's why for SPC-2 devices it only James shows up on 2TB devices because they force RC16. But we only do so if they have LBPME set. Generally they don't. The problem for hyperv was that LBPME was set. And because it claimed SBC-2 we did not check the LBP VPD page to see that it prefers UNMAP. I believe we did consider unconditionally checking for VPDs when LBPME was set but I seem to recall that it broke some device that had garbage in the READ CAPACITY(16) response. I'll see if I can locate the details. Otherwise I'm willing to entertain that idea. Well, your judgement: is this situation (support for UNMAP but not for WRITE_SAME) in what is effectively a RAID driver (hv drivers count as RAID) just a silly Microsoft one off? If so, we can go with your force VPD page patch. However, if we get any RAID drivers with strange discard support, we'll likely get the same problem ... I've got to say in the long run, it sounds likely we will given that RAID vendors are only marginally more competent than USB bridge vendors when it comes to following the spec. James -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
Rob == Elliott, Robert (Server Storage) elli...@hp.com writes: Rob WRITE SAME with the UNMAP bit set to one (and a few other Rob conditions) guarantees that the data is zeroed out, while the UNMAP Rob command is just a hint. They're not fully interchangeable. Which Rob semantics are implied by REQ_DISCARD and these functions? Voluntary deprovisioning of a block range. Rob One benefit of UNMAP is that UNMAP supports a list of discontiguous Rob LBA ranges, whereas WRITE SAME just supports one LBA range. Rob sd_setup_discard_cmnd is not taking advantage of this feature, Rob though. The block layer can only describe one contiguous block range in a request. My copy offload patches introduces the bi_special field that allows us to attach additional information to an I/O. I have experimented with doing that for discards to overcome the suck of DSM TRIM. Splitting and merging requests in MD/DM gets much more cumbersome, though. Rob Ideally, the block layer would merge multiple discards into one Rob UNMAP command if they're stuck on the request queue for a while, Rob like it merges adjacent reads and writes. We support merging contiguous smaller discard requests. We did discuss having a (separate) I/O scheduler to coalesce discontiguous discard requests. However, the attempts at this turned out to be pretty hideous. It also wasn't evident that it was worth the hassle. While UNMAP allows us to express large regions, DSM TRIM on the SATA side is limited to 32 MB per range. So in many cases we end up maxing out the payload capacity even with a single contiguous range. We expect LBP SCSI devices to queue commands. Being able to express multiple ranges in one shot is less critical in that case. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
James == James Bottomley jbottom...@parallels.com writes: James Well, your judgement: is this situation (support for UNMAP but James not for WRITE_SAME) in what is effectively a RAID driver (hv James drivers count as RAID) just a silly Microsoft one off? I only recall seeing one or two devices that supported LBP but not WRITE SAME w/UNMAP. James However, if we get any RAID drivers with strange discard support, James we'll likely get the same problem The LBP VPD page is mandatory now. It wasn't for the first couple of generations of devices that we still have to support. I think that if a vendor were to support LBP, adding the mandatory VPD page would be a given. And so far nobody has messed up the LBP VPD page contents. My main gripe about linking no_write_same and discard functionality is that the heuristics for the latter are already excessively complex thanks to having to support devices that predate the spec. I'm wary of adding another dimension to that. Also, linking the two use cases we can get into inconsistent states where no_write_same is set but the device does not support UNMAP and has LBPWS=1 and LBPWS10=1 set in the LBP VPD. I'll contemplate the LBPME = mandatory VPD lookup thing for bit. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message-
> From: driverdev-devel-boun...@linuxdriverproject.org [mailto:driverdev-
> devel-boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan
> Sent: Sunday, July 13, 2014 7:38 PM
> To: Martin K. Petersen
> Cc: linux-s...@vger.kernel.org; jasow...@redhat.com; linux-
> ker...@vger.kernel.org; sta...@vger.kernel.org; oher...@suse.com;
> h...@infradead.org; James Bottomley; a...@canonical.com;
> de...@linuxdriverproject.org
> Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
>
>
>
> > -Original Message-
> > From: KY Srinivasan
> > Sent: Sunday, July 13, 2014 11:50 AM
> > To: 'Martin K. Petersen'
> > Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
> > de...@linuxdriverproject.org; a...@canonical.com;
> > sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
> > jasow...@redhat.com
> > Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> >
> >
> >
> > > -Original Message-
> > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> > > Sent: Sunday, July 13, 2014 5:59 AM
> > > To: KY Srinivasan
> > > Cc: h...@infradead.org; James Bottomley;
> > > linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > > a...@canonical.com; sta...@vger.kernel.org;
> > > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
> > > WRITE_SAME_16
> > >
> > > >>>>> "KY" == KY Srinivasan writes:
> > >
> > > KY> Windows hosts do support UNMAP and set the field in the EVPD.
> > > KY> However, since the host advertises SPC-2 compliance, Linux does
> > > KY> not even query the VPD page.
> > >
> > > >> If we want to enable UNMAP in this case I'd prefer a blacklist
> > > >> entry than trying UNMAP despite the device not advertising it.
> > >
> > > I agree with that. We could do something like the patch below.
> > >
> > > However, I do think it's a good idea that you guys are looking into
> > > reporting SPC-3.
> >
> > Thanks Martin; this patch would address our present issues. I will get
> > some testing done and report back.
>
> How should we force the flag (BLIST_TRY_VPD_PAGES) to be set.
Once I force the flag set, it works. Thanks Martin.
K. Y
>
> K. Y
> >
> > Regards,
> >
> > K. Y
> > >
> > >
> > > SCSI: Add a blacklist flag which enables VPD page inquiries
> > >
> > > Despite supporting modern SCSI features some storage devices
> > > continue to claim conformance to an older version of the SPC spec.
> > > This is done for compatibility with legacy operating systems.
> > >
> > > Linux by default will not attempt to read VPD pages on devices that
> > > claim
> > > SPC-2 or older. Introduce a blacklist flag that can be used to
> > > trigger VPD page inquiries on devices that are known to support them.
> > >
> > > Reported-by: KY Srinivasan
> > > Signed-off-by: Martin K. Petersen
> > >
> > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > > index 4a6e4ba5a400..a5b1a224628a 100644
> > > --- a/drivers/scsi/scsi_scan.c
> > > +++ b/drivers/scsi/scsi_scan.c
> > > @@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device
> > > *sdev, unsigned char *inq_result,
> > >
> > > sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
> > >
> > > - if (*bflags & BLIST_SKIP_VPD_PAGES)
> > > + if (*bflags & BLIST_TRY_VPD_PAGES)
> > > + sdev->try_vpd_pages = 1;
> > > + else if (*bflags & BLIST_SKIP_VPD_PAGES)
> > > sdev->skip_vpd_pages = 1;
> > >
> > > transport_configure_device(>sdev_gendev);
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > > 87566b51fcf7..31d32b9077ca 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct
> > > scsi_disk *sdkp, unsigned char *buffer)
> > >
> > > static int sd_try_extended_inquiry(struct scsi_device *sdp) {
> > > + /* Attempt VPD inquiry if the device blacklist explicitly calls
> > > + * for it.
> > > + */
> > > + if (sdp->try_vpd_pages)
> > > + return 1;
> > > /*
> > >* Although VPD inquir
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message-
> From: KY Srinivasan
> Sent: Sunday, July 13, 2014 11:50 AM
> To: 'Martin K. Petersen'
> Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
> linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
>
>
>
> > -Original Message-
> > From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> > Sent: Sunday, July 13, 2014 5:59 AM
> > To: KY Srinivasan
> > Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
> > de...@linuxdriverproject.org; a...@canonical.com;
> > sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
> > jasow...@redhat.com
> > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> >
> > >>>>> "KY" == KY Srinivasan writes:
> >
> > KY> Windows hosts do support UNMAP and set the field in the EVPD.
> > KY> However, since the host advertises SPC-2 compliance, Linux does
> > KY> not even query the VPD page.
> >
> > >> If we want to enable UNMAP in this case I'd prefer a blacklist
> > >> entry than trying UNMAP despite the device not advertising it.
> >
> > I agree with that. We could do something like the patch below.
> >
> > However, I do think it's a good idea that you guys are looking into
> > reporting SPC-3.
>
> Thanks Martin; this patch would address our present issues. I will get some
> testing done and report back.
How should we force the flag (BLIST_TRY_VPD_PAGES) to be set.
K. Y
>
> Regards,
>
> K. Y
> >
> >
> > SCSI: Add a blacklist flag which enables VPD page inquiries
> >
> > Despite supporting modern SCSI features some storage devices continue
> > to claim conformance to an older version of the SPC spec. This is done
> > for compatibility with legacy operating systems.
> >
> > Linux by default will not attempt to read VPD pages on devices that
> > claim
> > SPC-2 or older. Introduce a blacklist flag that can be used to trigger
> > VPD page inquiries on devices that are known to support them.
> >
> > Reported-by: KY Srinivasan
> > Signed-off-by: Martin K. Petersen
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
> > 4a6e4ba5a400..a5b1a224628a 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> > unsigned char *inq_result,
> >
> > sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
> >
> > - if (*bflags & BLIST_SKIP_VPD_PAGES)
> > + if (*bflags & BLIST_TRY_VPD_PAGES)
> > + sdev->try_vpd_pages = 1;
> > + else if (*bflags & BLIST_SKIP_VPD_PAGES)
> > sdev->skip_vpd_pages = 1;
> >
> > transport_configure_device(>sdev_gendev);
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > 87566b51fcf7..31d32b9077ca 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk
> > *sdkp, unsigned char *buffer)
> >
> > static int sd_try_extended_inquiry(struct scsi_device *sdp) {
> > + /* Attempt VPD inquiry if the device blacklist explicitly calls
> > +* for it.
> > +*/
> > + if (sdp->try_vpd_pages)
> > + return 1;
> > /*
> > * Although VPD inquiries can go to SCSI-2 type devices,
> > * some USB ones crash on receiving them, and the pages diff --git
> > a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
> > 9aa38f7b303b..f579408620f0 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -155,6 +155,7 @@ struct scsi_device {
> > unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08
> > */
> > unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
> > */
> > unsigned skip_vpd_pages:1; /* do not read VPD pages */
> > + unsigned try_vpd_pages:1; /* attempt to read VPD pages */
> > unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
> 0x3f
> > */
> > unsigned no_start_on_add:1; /* do not issue start on add */
> > unsigned allow_restart:1; /* issue START_UNIT in error handler */
> > diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> > index 8670c04e199e
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Sunday, July 13, 2014 5:59 AM
> To: KY Srinivasan
> Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
> linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
>
> >>>>> "KY" == KY Srinivasan writes:
>
> KY> Windows hosts do support UNMAP and set the field in the EVPD.
> KY> However, since the host advertises SPC-2 compliance, Linux does not
> KY> even query the VPD page.
>
> >> If we want to enable UNMAP in this case I'd prefer a blacklist entry
> >> than trying UNMAP despite the device not advertising it.
>
> I agree with that. We could do something like the patch below.
>
> However, I do think it's a good idea that you guys are looking into reporting
> SPC-3.
Thanks Martin; this patch would address our present issues. I will get some
testing done
and report back.
Regards,
K. Y
>
>
> SCSI: Add a blacklist flag which enables VPD page inquiries
>
> Despite supporting modern SCSI features some storage devices continue to
> claim conformance to an older version of the SPC spec. This is done for
> compatibility with legacy operating systems.
>
> Linux by default will not attempt to read VPD pages on devices that claim
> SPC-2 or older. Introduce a blacklist flag that can be used to trigger VPD
> page
> inquiries on devices that are known to support them.
>
> Reported-by: KY Srinivasan
> Signed-off-by: Martin K. Petersen
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
> 4a6e4ba5a400..a5b1a224628a 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> unsigned char *inq_result,
>
> sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>
> - if (*bflags & BLIST_SKIP_VPD_PAGES)
> + if (*bflags & BLIST_TRY_VPD_PAGES)
> + sdev->try_vpd_pages = 1;
> + else if (*bflags & BLIST_SKIP_VPD_PAGES)
> sdev->skip_vpd_pages = 1;
>
> transport_configure_device(>sdev_gendev);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> 87566b51fcf7..31d32b9077ca 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk
> *sdkp, unsigned char *buffer)
>
> static int sd_try_extended_inquiry(struct scsi_device *sdp) {
> + /* Attempt VPD inquiry if the device blacklist explicitly calls
> + * for it.
> + */
> + if (sdp->try_vpd_pages)
> + return 1;
> /*
>* Although VPD inquiries can go to SCSI-2 type devices,
>* some USB ones crash on receiving them, and the pages diff --git
> a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
> 9aa38f7b303b..f579408620f0 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -155,6 +155,7 @@ struct scsi_device {
> unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08
> */
> unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
> */
> unsigned skip_vpd_pages:1; /* do not read VPD pages */
> + unsigned try_vpd_pages:1; /* attempt to read VPD pages */
> unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
> 0x3f */
> unsigned no_start_on_add:1; /* do not issue start on add */
> unsigned allow_restart:1; /* issue START_UNIT in error handler */
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h index
> 8670c04e199e..1fdd6fc5492b 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -34,4 +34,5 @@
> #define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages
> */
> #define BLIST_SCSI3LUN 0x800 /* Scan more than 256
> LUNs
>for sequential scan */
> +#define BLIST_TRY_VPD_PAGES 0x1000 /* Attempt to read VPD
> pages */
> #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "KY" == KY Srinivasan writes:
KY> Windows hosts do support UNMAP and set the field in the
KY> EVPD. However, since the host advertises SPC-2 compliance, Linux
KY> does not even query the VPD page.
>> If we want to enable UNMAP in this case I'd prefer a blacklist entry
>> than trying UNMAP despite the device not advertising it.
I agree with that. We could do something like the patch below.
However, I do think it's a good idea that you guys are looking into
reporting SPC-3.
SCSI: Add a blacklist flag which enables VPD page inquiries
Despite supporting modern SCSI features some storage devices continue to
claim conformance to an older version of the SPC spec. This is done for
compatibility with legacy operating systems.
Linux by default will not attempt to read VPD pages on devices that
claim SPC-2 or older. Introduce a blacklist flag that can be used to
trigger VPD page inquiries on devices that are known to support them.
Reported-by: KY Srinivasan
Signed-off-by: Martin K. Petersen
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4a6e4ba5a400..a5b1a224628a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned
char *inq_result,
sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
- if (*bflags & BLIST_SKIP_VPD_PAGES)
+ if (*bflags & BLIST_TRY_VPD_PAGES)
+ sdev->try_vpd_pages = 1;
+ else if (*bflags & BLIST_SKIP_VPD_PAGES)
sdev->skip_vpd_pages = 1;
transport_configure_device(>sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 87566b51fcf7..31d32b9077ca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk *sdkp,
unsigned char *buffer)
static int sd_try_extended_inquiry(struct scsi_device *sdp)
{
+ /* Attempt VPD inquiry if the device blacklist explicitly calls
+* for it.
+*/
+ if (sdp->try_vpd_pages)
+ return 1;
/*
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9aa38f7b303b..f579408620f0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08 */
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f */
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned try_vpd_pages:1; /* attempt to read VPD pages */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 8670c04e199e..1fdd6fc5492b 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,4 +34,5 @@
#define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages */
#define BLIST_SCSI3LUN 0x800 /* Scan more than 256 LUNs
for sequential scan */
+#define BLIST_TRY_VPD_PAGES0x1000 /* Attempt to read VPD pages */
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
KY == KY Srinivasan k...@microsoft.com writes:
KY Windows hosts do support UNMAP and set the field in the
KY EVPD. However, since the host advertises SPC-2 compliance, Linux
KY does not even query the VPD page.
If we want to enable UNMAP in this case I'd prefer a blacklist entry
than trying UNMAP despite the device not advertising it.
I agree with that. We could do something like the patch below.
However, I do think it's a good idea that you guys are looking into
reporting SPC-3.
SCSI: Add a blacklist flag which enables VPD page inquiries
Despite supporting modern SCSI features some storage devices continue to
claim conformance to an older version of the SPC spec. This is done for
compatibility with legacy operating systems.
Linux by default will not attempt to read VPD pages on devices that
claim SPC-2 or older. Introduce a blacklist flag that can be used to
trigger VPD page inquiries on devices that are known to support them.
Reported-by: KY Srinivasan k...@microsoft.com
Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4a6e4ba5a400..a5b1a224628a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned
char *inq_result,
sdev-eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
- if (*bflags BLIST_SKIP_VPD_PAGES)
+ if (*bflags BLIST_TRY_VPD_PAGES)
+ sdev-try_vpd_pages = 1;
+ else if (*bflags BLIST_SKIP_VPD_PAGES)
sdev-skip_vpd_pages = 1;
transport_configure_device(sdev-sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 87566b51fcf7..31d32b9077ca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk *sdkp,
unsigned char *buffer)
static int sd_try_extended_inquiry(struct scsi_device *sdp)
{
+ /* Attempt VPD inquiry if the device blacklist explicitly calls
+* for it.
+*/
+ if (sdp-try_vpd_pages)
+ return 1;
/*
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9aa38f7b303b..f579408620f0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08 */
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f */
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned try_vpd_pages:1; /* attempt to read VPD pages */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 8670c04e199e..1fdd6fc5492b 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,4 +34,5 @@
#define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages */
#define BLIST_SCSI3LUN 0x800 /* Scan more than 256 LUNs
for sequential scan */
+#define BLIST_TRY_VPD_PAGES0x1000 /* Attempt to read VPD pages */
#endif
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Sunday, July 13, 2014 5:59 AM
To: KY Srinivasan
Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
KY == KY Srinivasan k...@microsoft.com writes:
KY Windows hosts do support UNMAP and set the field in the EVPD.
KY However, since the host advertises SPC-2 compliance, Linux does not
KY even query the VPD page.
If we want to enable UNMAP in this case I'd prefer a blacklist entry
than trying UNMAP despite the device not advertising it.
I agree with that. We could do something like the patch below.
However, I do think it's a good idea that you guys are looking into reporting
SPC-3.
Thanks Martin; this patch would address our present issues. I will get some
testing done
and report back.
Regards,
K. Y
SCSI: Add a blacklist flag which enables VPD page inquiries
Despite supporting modern SCSI features some storage devices continue to
claim conformance to an older version of the SPC spec. This is done for
compatibility with legacy operating systems.
Linux by default will not attempt to read VPD pages on devices that claim
SPC-2 or older. Introduce a blacklist flag that can be used to trigger VPD
page
inquiries on devices that are known to support them.
Reported-by: KY Srinivasan k...@microsoft.com
Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
4a6e4ba5a400..a5b1a224628a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
unsigned char *inq_result,
sdev-eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
- if (*bflags BLIST_SKIP_VPD_PAGES)
+ if (*bflags BLIST_TRY_VPD_PAGES)
+ sdev-try_vpd_pages = 1;
+ else if (*bflags BLIST_SKIP_VPD_PAGES)
sdev-skip_vpd_pages = 1;
transport_configure_device(sdev-sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
87566b51fcf7..31d32b9077ca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk
*sdkp, unsigned char *buffer)
static int sd_try_extended_inquiry(struct scsi_device *sdp) {
+ /* Attempt VPD inquiry if the device blacklist explicitly calls
+ * for it.
+ */
+ if (sdp-try_vpd_pages)
+ return 1;
/*
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages diff --git
a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
9aa38f7b303b..f579408620f0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08
*/
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
*/
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned try_vpd_pages:1; /* attempt to read VPD pages */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
0x3f */
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h index
8670c04e199e..1fdd6fc5492b 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,4 +34,5 @@
#define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages
*/
#define BLIST_SCSI3LUN 0x800 /* Scan more than 256
LUNs
for sequential scan */
+#define BLIST_TRY_VPD_PAGES 0x1000 /* Attempt to read VPD
pages */
#endif
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: KY Srinivasan
Sent: Sunday, July 13, 2014 11:50 AM
To: 'Martin K. Petersen'
Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Sunday, July 13, 2014 5:59 AM
To: KY Srinivasan
Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; a...@canonical.com;
sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
KY == KY Srinivasan k...@microsoft.com writes:
KY Windows hosts do support UNMAP and set the field in the EVPD.
KY However, since the host advertises SPC-2 compliance, Linux does
KY not even query the VPD page.
If we want to enable UNMAP in this case I'd prefer a blacklist
entry than trying UNMAP despite the device not advertising it.
I agree with that. We could do something like the patch below.
However, I do think it's a good idea that you guys are looking into
reporting SPC-3.
Thanks Martin; this patch would address our present issues. I will get some
testing done and report back.
How should we force the flag (BLIST_TRY_VPD_PAGES) to be set.
K. Y
Regards,
K. Y
SCSI: Add a blacklist flag which enables VPD page inquiries
Despite supporting modern SCSI features some storage devices continue
to claim conformance to an older version of the SPC spec. This is done
for compatibility with legacy operating systems.
Linux by default will not attempt to read VPD pages on devices that
claim
SPC-2 or older. Introduce a blacklist flag that can be used to trigger
VPD page inquiries on devices that are known to support them.
Reported-by: KY Srinivasan k...@microsoft.com
Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
4a6e4ba5a400..a5b1a224628a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
unsigned char *inq_result,
sdev-eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
- if (*bflags BLIST_SKIP_VPD_PAGES)
+ if (*bflags BLIST_TRY_VPD_PAGES)
+ sdev-try_vpd_pages = 1;
+ else if (*bflags BLIST_SKIP_VPD_PAGES)
sdev-skip_vpd_pages = 1;
transport_configure_device(sdev-sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
87566b51fcf7..31d32b9077ca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk
*sdkp, unsigned char *buffer)
static int sd_try_extended_inquiry(struct scsi_device *sdp) {
+ /* Attempt VPD inquiry if the device blacklist explicitly calls
+* for it.
+*/
+ if (sdp-try_vpd_pages)
+ return 1;
/*
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages diff --git
a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
9aa38f7b303b..f579408620f0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08
*/
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
*/
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned try_vpd_pages:1; /* attempt to read VPD pages */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
0x3f
*/
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 8670c04e199e..1fdd6fc5492b 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,4 +34,5 @@
#define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages
*/
#define BLIST_SCSI3LUN 0x800 /* Scan more than 256
LUNs
for sequential scan */
+#define BLIST_TRY_VPD_PAGES0x1000 /* Attempt to read VPD
pages */
#endif
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: driverdev-devel-boun...@linuxdriverproject.org [mailto:driverdev-
devel-boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan
Sent: Sunday, July 13, 2014 7:38 PM
To: Martin K. Petersen
Cc: linux-s...@vger.kernel.org; jasow...@redhat.com; linux-
ker...@vger.kernel.org; sta...@vger.kernel.org; oher...@suse.com;
h...@infradead.org; James Bottomley; a...@canonical.com;
de...@linuxdriverproject.org
Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: KY Srinivasan
Sent: Sunday, July 13, 2014 11:50 AM
To: 'Martin K. Petersen'
Cc: h...@infradead.org; James Bottomley; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; a...@canonical.com;
sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
jasow...@redhat.com
Subject: RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Sunday, July 13, 2014 5:59 AM
To: KY Srinivasan
Cc: h...@infradead.org; James Bottomley;
linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
a...@canonical.com; sta...@vger.kernel.org;
linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
WRITE_SAME_16
KY == KY Srinivasan k...@microsoft.com writes:
KY Windows hosts do support UNMAP and set the field in the EVPD.
KY However, since the host advertises SPC-2 compliance, Linux does
KY not even query the VPD page.
If we want to enable UNMAP in this case I'd prefer a blacklist
entry than trying UNMAP despite the device not advertising it.
I agree with that. We could do something like the patch below.
However, I do think it's a good idea that you guys are looking into
reporting SPC-3.
Thanks Martin; this patch would address our present issues. I will get
some testing done and report back.
How should we force the flag (BLIST_TRY_VPD_PAGES) to be set.
Once I force the flag set, it works. Thanks Martin.
K. Y
K. Y
Regards,
K. Y
SCSI: Add a blacklist flag which enables VPD page inquiries
Despite supporting modern SCSI features some storage devices
continue to claim conformance to an older version of the SPC spec.
This is done for compatibility with legacy operating systems.
Linux by default will not attempt to read VPD pages on devices that
claim
SPC-2 or older. Introduce a blacklist flag that can be used to
trigger VPD page inquiries on devices that are known to support them.
Reported-by: KY Srinivasan k...@microsoft.com
Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4a6e4ba5a400..a5b1a224628a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device
*sdev, unsigned char *inq_result,
sdev-eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
- if (*bflags BLIST_SKIP_VPD_PAGES)
+ if (*bflags BLIST_TRY_VPD_PAGES)
+ sdev-try_vpd_pages = 1;
+ else if (*bflags BLIST_SKIP_VPD_PAGES)
sdev-skip_vpd_pages = 1;
transport_configure_device(sdev-sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
87566b51fcf7..31d32b9077ca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct
scsi_disk *sdkp, unsigned char *buffer)
static int sd_try_extended_inquiry(struct scsi_device *sdp) {
+ /* Attempt VPD inquiry if the device blacklist explicitly calls
+ * for it.
+ */
+ if (sdp-try_vpd_pages)
+ return 1;
/*
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages diff --git
a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
9aa38f7b303b..f579408620f0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08
*/
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
*/
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned try_vpd_pages:1; /* attempt to read VPD pages */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
0x3f
*/
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h
b/include/scsi/scsi_devinfo.h index 8670c04e199e..1fdd6fc5492b
100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,4 +34,5 @@
#define BLIST_SKIP_VPD_PAGES 0x400
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message- > From: Martin K. Petersen [mailto:martin.peter...@oracle.com] > Sent: Friday, July 11, 2014 5:54 AM > To: h...@infradead.org > Cc: James Bottomley; KY Srinivasan; linux-kernel@vger.kernel.org; > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > >>>>> "hch" == hch@infradead org writes: > > (Back from vacation: Bear with me while I'm catching up on two weeks of > linux-scsi stuff...) > > hch> I think the problem is a differnet one. If we have the logical > hch> provisioning EVPD it configures what method to use, but if we don't > hch> have one we simply check for a max unmap blocks field, and if > hch> that's not present use WRITE SAME. > > Yeah, that was done to accommodate the devices out there that predate the > LBP VPD. There sadly are several still. And it's hard to motivate people to > update their storage array firmware even when updates are readily available. > > hch> The patch checks the no_write_same flag before doing that, for > hch> which we also have to do the write_same setup before the discard > hch> setup in sd_revalidate_disk. > > The no_write_same was introduced to disable the REQ_WRITE_SAME use > case where we have no INQUIRY/READ CAPACITY/VPD flags to key off of to > determine whether it is safe to send the commands. > > no_write_same does not preclude the REQ_DISCARD variants of > WRITE_SAME. Those are gated by the discard heuristics exclusively. > > So first of all I'd like to know whether it's a WRITE SAME(16) or a WRITE > SAME(16) with the UNMAP bit set that's coming down. > > hch> Ky: does hyperv support UNMAP? If so any idea why it doesn't set > hch> the maximum unmap block count field in the EVPD? > > We shouldn't be sending down WRITE SAME(16) with the UNMAP bit set > unless the device sets LBPME=1 in the READ CAPACITY(16) response. > > So what does the storsvc report as its thin provisioning capabilities? Given that our current Host (ws2012 r2) advertises SPC-2 compliance, Linux does not even query this page. We support UNMAP. We just prototyped a host where we advertised SPC-3 compliance and Linux queries the EVPD page and does not use WRITE_SAME_16 K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message-
> From: h...@infradead.org [mailto:h...@infradead.org]
> Sent: Thursday, July 10, 2014 11:32 PM
> To: James Bottomley
> Cc: KY Srinivasan; linux-kernel@vger.kernel.org; m...@mkp.net;
> h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com;
> sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
> jasow...@redhat.com
> Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
>
> On Wed, Jul 09, 2014 at 10:27:24PM +, James Bottomley wrote:
> > If we fix it at source, why would there be any need to filter? That's
> > the reason the no_write_same flag was introduced. If we can find and
> > fix the bug, it can go back into the stable trees as a bug fix, hence
> > nothing should ever emit write_same(10 or 16) and additional driver
> > code is redundant (and counter productive, since if this ever breaks
> > again you're our best canary).
> >
> > This looks like it might be the problem but Martin should confirm (I
> > think the problem comes to us from the RC16 code which unconditionally
> > sets WS16).
>
> I think the problem is a differnet one. If we have the logical provisioning
> EVPD it configures what method to use, but if we don't have one we simply
> check for a max unmap blocks field, and if that's not present use WRITE
> SAME.
>
> The patch checks the no_write_same flag before doing that, for which we
> also have to do the write_same setup before the discard setup in
> sd_revalidate_disk.
>
> Ky: does hyperv support UNMAP? If so any idea why it doesn't set the
> maximum unmap block count field in the EVPD?
Windows hosts do support UNMAP and set the field in the EVPD. However, since
the host
advertises SPC-2 compliance, Linux does not even query the VPD page.
>
> If we want to enable UNMAP in this case I'd prefer a blacklist entry than
> trying UNMAP despite the device not advertising it.
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ba756b1..fbccfd2
> 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2614,9 +2614,10 @@ static void sd_read_block_limits(struct scsi_disk
> *sdkp)
>
> if (sdkp->max_unmap_blocks)
> sd_config_discard(sdkp, SD_LBP_UNMAP);
> - else
> + else if (!sdkp->device->no_write_same)
> sd_config_discard(sdkp, SD_LBP_WS16);
> -
> + else
> + sd_config_discard(sdkp, SD_LBP_DISABLE);
> } else {/* LBP VPD page tells us what to use */
>
> if (sdkp->lbpu && sdkp->max_unmap_blocks) @@ -
> 2766,6 +2767,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>*/
> if (sdkp->media_present) {
> sd_read_capacity(sdkp, buffer);
> + sd_read_write_same(sdkp, buffer);
>
> if (sd_try_extended_inquiry(sdp)) {
> sd_read_block_provisioning(sdkp);
> @@ -2776,7 +2778,6 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sd_read_write_protect_flag(sdkp, buffer);
> sd_read_cache_type(sdkp, buffer);
> sd_read_app_tag_own(sdkp, buffer);
> - sd_read_write_same(sdkp, buffer);
> }
>
> sdkp->first_scan = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> "hch" == hch@infradead org writes: (Back from vacation: Bear with me while I'm catching up on two weeks of linux-scsi stuff...) hch> I think the problem is a differnet one. If we have the logical hch> provisioning EVPD it configures what method to use, but if we don't hch> have one we simply check for a max unmap blocks field, and if hch> that's not present use WRITE SAME. Yeah, that was done to accommodate the devices out there that predate the LBP VPD. There sadly are several still. And it's hard to motivate people to update their storage array firmware even when updates are readily available. hch> The patch checks the no_write_same flag before doing that, for hch> which we also have to do the write_same setup before the discard hch> setup in sd_revalidate_disk. The no_write_same was introduced to disable the REQ_WRITE_SAME use case where we have no INQUIRY/READ CAPACITY/VPD flags to key off of to determine whether it is safe to send the commands. no_write_same does not preclude the REQ_DISCARD variants of WRITE_SAME. Those are gated by the discard heuristics exclusively. So first of all I'd like to know whether it's a WRITE SAME(16) or a WRITE SAME(16) with the UNMAP bit set that's coming down. hch> Ky: does hyperv support UNMAP? If so any idea why it doesn't set hch> the maximum unmap block count field in the EVPD? We shouldn't be sending down WRITE SAME(16) with the UNMAP bit set unless the device sets LBPME=1 in the READ CAPACITY(16) response. So what does the storsvc report as its thin provisioning capabilities? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 09, 2014 at 10:27:24PM +, James Bottomley wrote:
> If we fix it at source, why would there be any need to filter? That's
> the reason the no_write_same flag was introduced. If we can find and
> fix the bug, it can go back into the stable trees as a bug fix, hence
> nothing should ever emit write_same(10 or 16) and additional driver code
> is redundant (and counter productive, since if this ever breaks again
> you're our best canary).
>
> This looks like it might be the problem but Martin should confirm (I
> think the problem comes to us from the RC16 code which unconditionally
> sets WS16).
I think the problem is a differnet one. If we have the logical
provisioning EVPD it configures what method to use, but if we don't have
one we simply check for a max unmap blocks field, and if that's not
present use WRITE SAME.
The patch checks the no_write_same flag before doing that, for which
we also have to do the write_same setup before the discard setup
in sd_revalidate_disk.
Ky: does hyperv support UNMAP? If so any idea why it doesn't set
the maximum unmap block count field in the EVPD?
If we want to enable UNMAP in this case I'd prefer a blacklist entry
than trying UNMAP despite the device not advertising it.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ba756b1..fbccfd2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2614,9 +2614,10 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
if (sdkp->max_unmap_blocks)
sd_config_discard(sdkp, SD_LBP_UNMAP);
- else
+ else if (!sdkp->device->no_write_same)
sd_config_discard(sdkp, SD_LBP_WS16);
-
+ else
+ sd_config_discard(sdkp, SD_LBP_DISABLE);
} else {/* LBP VPD page tells us what to use */
if (sdkp->lbpu && sdkp->max_unmap_blocks)
@@ -2766,6 +2767,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
*/
if (sdkp->media_present) {
sd_read_capacity(sdkp, buffer);
+ sd_read_write_same(sdkp, buffer);
if (sd_try_extended_inquiry(sdp)) {
sd_read_block_provisioning(sdkp);
@@ -2776,7 +2778,6 @@ static int sd_revalidate_disk(struct gendisk *disk)
sd_read_write_protect_flag(sdkp, buffer);
sd_read_cache_type(sdkp, buffer);
sd_read_app_tag_own(sdkp, buffer);
- sd_read_write_same(sdkp, buffer);
}
sdkp->first_scan = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 09, 2014 at 10:27:24PM +, James Bottomley wrote:
If we fix it at source, why would there be any need to filter? That's
the reason the no_write_same flag was introduced. If we can find and
fix the bug, it can go back into the stable trees as a bug fix, hence
nothing should ever emit write_same(10 or 16) and additional driver code
is redundant (and counter productive, since if this ever breaks again
you're our best canary).
This looks like it might be the problem but Martin should confirm (I
think the problem comes to us from the RC16 code which unconditionally
sets WS16).
I think the problem is a differnet one. If we have the logical
provisioning EVPD it configures what method to use, but if we don't have
one we simply check for a max unmap blocks field, and if that's not
present use WRITE SAME.
The patch checks the no_write_same flag before doing that, for which
we also have to do the write_same setup before the discard setup
in sd_revalidate_disk.
Ky: does hyperv support UNMAP? If so any idea why it doesn't set
the maximum unmap block count field in the EVPD?
If we want to enable UNMAP in this case I'd prefer a blacklist entry
than trying UNMAP despite the device not advertising it.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ba756b1..fbccfd2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2614,9 +2614,10 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
if (sdkp-max_unmap_blocks)
sd_config_discard(sdkp, SD_LBP_UNMAP);
- else
+ else if (!sdkp-device-no_write_same)
sd_config_discard(sdkp, SD_LBP_WS16);
-
+ else
+ sd_config_discard(sdkp, SD_LBP_DISABLE);
} else {/* LBP VPD page tells us what to use */
if (sdkp-lbpu sdkp-max_unmap_blocks)
@@ -2766,6 +2767,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
*/
if (sdkp-media_present) {
sd_read_capacity(sdkp, buffer);
+ sd_read_write_same(sdkp, buffer);
if (sd_try_extended_inquiry(sdp)) {
sd_read_block_provisioning(sdkp);
@@ -2776,7 +2778,6 @@ static int sd_revalidate_disk(struct gendisk *disk)
sd_read_write_protect_flag(sdkp, buffer);
sd_read_cache_type(sdkp, buffer);
sd_read_app_tag_own(sdkp, buffer);
- sd_read_write_same(sdkp, buffer);
}
sdkp-first_scan = 0;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
hch == hch@infradead org h...@infradead.org writes: (Back from vacation: Bear with me while I'm catching up on two weeks of linux-scsi stuff...) hch I think the problem is a differnet one. If we have the logical hch provisioning EVPD it configures what method to use, but if we don't hch have one we simply check for a max unmap blocks field, and if hch that's not present use WRITE SAME. Yeah, that was done to accommodate the devices out there that predate the LBP VPD. There sadly are several still. And it's hard to motivate people to update their storage array firmware even when updates are readily available. hch The patch checks the no_write_same flag before doing that, for hch which we also have to do the write_same setup before the discard hch setup in sd_revalidate_disk. The no_write_same was introduced to disable the REQ_WRITE_SAME use case where we have no INQUIRY/READ CAPACITY/VPD flags to key off of to determine whether it is safe to send the commands. no_write_same does not preclude the REQ_DISCARD variants of WRITE_SAME. Those are gated by the discard heuristics exclusively. So first of all I'd like to know whether it's a WRITE SAME(16) or a WRITE SAME(16) with the UNMAP bit set that's coming down. hch Ky: does hyperv support UNMAP? If so any idea why it doesn't set hch the maximum unmap block count field in the EVPD? We shouldn't be sending down WRITE SAME(16) with the UNMAP bit set unless the device sets LBPME=1 in the READ CAPACITY(16) response. So what does the storsvc report as its thin provisioning capabilities? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: h...@infradead.org [mailto:h...@infradead.org]
Sent: Thursday, July 10, 2014 11:32 PM
To: James Bottomley
Cc: KY Srinivasan; linux-kernel@vger.kernel.org; m...@mkp.net;
h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com;
sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com;
jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 09, 2014 at 10:27:24PM +, James Bottomley wrote:
If we fix it at source, why would there be any need to filter? That's
the reason the no_write_same flag was introduced. If we can find and
fix the bug, it can go back into the stable trees as a bug fix, hence
nothing should ever emit write_same(10 or 16) and additional driver
code is redundant (and counter productive, since if this ever breaks
again you're our best canary).
This looks like it might be the problem but Martin should confirm (I
think the problem comes to us from the RC16 code which unconditionally
sets WS16).
I think the problem is a differnet one. If we have the logical provisioning
EVPD it configures what method to use, but if we don't have one we simply
check for a max unmap blocks field, and if that's not present use WRITE
SAME.
The patch checks the no_write_same flag before doing that, for which we
also have to do the write_same setup before the discard setup in
sd_revalidate_disk.
Ky: does hyperv support UNMAP? If so any idea why it doesn't set the
maximum unmap block count field in the EVPD?
Windows hosts do support UNMAP and set the field in the EVPD. However, since
the host
advertises SPC-2 compliance, Linux does not even query the VPD page.
If we want to enable UNMAP in this case I'd prefer a blacklist entry than
trying UNMAP despite the device not advertising it.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ba756b1..fbccfd2
100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2614,9 +2614,10 @@ static void sd_read_block_limits(struct scsi_disk
*sdkp)
if (sdkp-max_unmap_blocks)
sd_config_discard(sdkp, SD_LBP_UNMAP);
- else
+ else if (!sdkp-device-no_write_same)
sd_config_discard(sdkp, SD_LBP_WS16);
-
+ else
+ sd_config_discard(sdkp, SD_LBP_DISABLE);
} else {/* LBP VPD page tells us what to use */
if (sdkp-lbpu sdkp-max_unmap_blocks) @@ -
2766,6 +2767,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
*/
if (sdkp-media_present) {
sd_read_capacity(sdkp, buffer);
+ sd_read_write_same(sdkp, buffer);
if (sd_try_extended_inquiry(sdp)) {
sd_read_block_provisioning(sdkp);
@@ -2776,7 +2778,6 @@ static int sd_revalidate_disk(struct gendisk *disk)
sd_read_write_protect_flag(sdkp, buffer);
sd_read_cache_type(sdkp, buffer);
sd_read_app_tag_own(sdkp, buffer);
- sd_read_write_same(sdkp, buffer);
}
sdkp-first_scan = 0;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message- From: Martin K. Petersen [mailto:martin.peter...@oracle.com] Sent: Friday, July 11, 2014 5:54 AM To: h...@infradead.org Cc: James Bottomley; KY Srinivasan; linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 hch == hch@infradead org h...@infradead.org writes: (Back from vacation: Bear with me while I'm catching up on two weeks of linux-scsi stuff...) hch I think the problem is a differnet one. If we have the logical hch provisioning EVPD it configures what method to use, but if we don't hch have one we simply check for a max unmap blocks field, and if hch that's not present use WRITE SAME. Yeah, that was done to accommodate the devices out there that predate the LBP VPD. There sadly are several still. And it's hard to motivate people to update their storage array firmware even when updates are readily available. hch The patch checks the no_write_same flag before doing that, for hch which we also have to do the write_same setup before the discard hch setup in sd_revalidate_disk. The no_write_same was introduced to disable the REQ_WRITE_SAME use case where we have no INQUIRY/READ CAPACITY/VPD flags to key off of to determine whether it is safe to send the commands. no_write_same does not preclude the REQ_DISCARD variants of WRITE_SAME. Those are gated by the discard heuristics exclusively. So first of all I'd like to know whether it's a WRITE SAME(16) or a WRITE SAME(16) with the UNMAP bit set that's coming down. hch Ky: does hyperv support UNMAP? If so any idea why it doesn't set hch the maximum unmap block count field in the EVPD? We shouldn't be sending down WRITE SAME(16) with the UNMAP bit set unless the device sets LBPME=1 in the READ CAPACITY(16) response. So what does the storsvc report as its thin provisioning capabilities? Given that our current Host (ws2012 r2) advertises SPC-2 compliance, Linux does not even query this page. We support UNMAP. We just prototyped a host where we advertised SPC-3 compliance and Linux queries the EVPD page and does not use WRITE_SAME_16 K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Thu, 2014-07-10 at 21:02 +, KY Srinivasan wrote: > > > -Original Message- > > From: James Bottomley [mailto:jbottom...@parallels.com] > > Sent: Wednesday, July 9, 2014 3:27 PM > > To: KY Srinivasan > > Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org; > > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; > > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > > > On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote: > > > > > > > -Original Message- > > > > From: James Bottomley [mailto:jbottom...@parallels.com] > > > > Sent: Wednesday, July 9, 2014 12:57 PM > > > > To: KY Srinivasan > > > > Cc: linux-kernel@vger.kernel.org; h...@infradead.org; > > > > de...@linuxdriverproject.org; a...@canonical.com; > > > > sta...@vger.kernel.org; linux-s...@vger.kernel.org; > > > > oher...@suse.com; jasow...@redhat.com > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > > WRITE_SAME_16 > > > > > > > > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > > > > > > > > > -Original Message- > > > > > > From: Christoph Hellwig [mailto:h...@infradead.org] > > > > > > Sent: Wednesday, July 9, 2014 1:43 AM > > > > > > To: KY Srinivasan > > > > > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > > > > > > oher...@suse.com; jbottom...@parallels.com; > > jasow...@redhat.com; > > > > > > a...@canonical.com; linux-s...@vger.kernel.org; > > > > > > sta...@vger.kernel.org > > > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > > > > WRITE_SAME_16 > > > > > > > > > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > > > > > Host does not handle WRITE_SAME_16; filter this command out. > > > > > > > This patch is required to handle large devices (greater than 2 TB > > disks). > > > > > > > > > > > > Storvsc already sets the no_write_same flag, where is the > > > > > > command coming from? > > > > > > > > > > In spite of this flag, I see WRITE_SAME_16 being issued when I > > > > > format a device bigger than 2 TB; I tried both xfs and ext4. > > > > > Windows hosts currently do not handle unsupported commands > > > > > correctly - The information returned is not sufficient to effect > > > > > recovery > > in the Linux guest. > > > > While this may be addressed in future hosts, this patch fixes the > > > > problem. > > > > > > > > What Christoph means is that this looks like a bug somewhere in SCSI > > itself. > > > > That means we need to find it and kill it, not add workarounds to > > > > every driver that sets no_write_same ... > > > > > > James, > > > > > > I will try to isolate this issue in the SCSI stack. If it is ok with > > > you guys, I would still want to filter WRITE_SAME_16 (as we currently > > > do WRITE_SAME) in our driver since this would address the problem for a > > large number of customers on our platform. > > > > If we fix it at source, why would there be any need to filter? That's the > > reason the no_write_same flag was introduced. If we can find and fix the > > bug, it can go back into the stable trees as a bug fix, hence nothing should > > ever emit write_same(10 or 16) and additional driver code is redundant (and > > counter productive, since if this ever breaks again you're our best canary). > > > > This looks like it might be the problem but Martin should confirm (I think > > the > > problem comes to us from the RC16 code which unconditionally sets WS16). > > > > James > > James, > > This patch works for me; are you planning on committing this patch. OK, if we go with this we can add your tested by. It's not going in until Martin takes a look because there may be a better way of doing this. James
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Wednesday, July 9, 2014 3:27 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org;
> de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
> linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
>
> On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley [mailto:jbottom...@parallels.com]
> > > Sent: Wednesday, July 9, 2014 12:57 PM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; h...@infradead.org;
> > > de...@linuxdriverproject.org; a...@canonical.com;
> > > sta...@vger.kernel.org; linux-s...@vger.kernel.org;
> > > oher...@suse.com; jasow...@redhat.com
> > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
> > > WRITE_SAME_16
> > >
> > > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Christoph Hellwig [mailto:h...@infradead.org]
> > > > > Sent: Wednesday, July 9, 2014 1:43 AM
> > > > > To: KY Srinivasan
> > > > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > > > > oher...@suse.com; jbottom...@parallels.com;
> jasow...@redhat.com;
> > > > > a...@canonical.com; linux-s...@vger.kernel.org;
> > > > > sta...@vger.kernel.org
> > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
> > > > > WRITE_SAME_16
> > > > >
> > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote:
> > > > > > Host does not handle WRITE_SAME_16; filter this command out.
> > > > > > This patch is required to handle large devices (greater than 2 TB
> disks).
> > > > >
> > > > > Storvsc already sets the no_write_same flag, where is the
> > > > > command coming from?
> > > >
> > > > In spite of this flag, I see WRITE_SAME_16 being issued when I
> > > > format a device bigger than 2 TB; I tried both xfs and ext4.
> > > > Windows hosts currently do not handle unsupported commands
> > > > correctly - The information returned is not sufficient to effect
> > > > recovery
> in the Linux guest.
> > > While this may be addressed in future hosts, this patch fixes the problem.
> > >
> > > What Christoph means is that this looks like a bug somewhere in SCSI
> itself.
> > > That means we need to find it and kill it, not add workarounds to
> > > every driver that sets no_write_same ...
> >
> > James,
> >
> > I will try to isolate this issue in the SCSI stack. If it is ok with
> > you guys, I would still want to filter WRITE_SAME_16 (as we currently
> > do WRITE_SAME) in our driver since this would address the problem for a
> large number of customers on our platform.
>
> If we fix it at source, why would there be any need to filter? That's the
> reason the no_write_same flag was introduced. If we can find and fix the
> bug, it can go back into the stable trees as a bug fix, hence nothing should
> ever emit write_same(10 or 16) and additional driver code is redundant (and
> counter productive, since if this ever breaks again you're our best canary).
>
> This looks like it might be the problem but Martin should confirm (I think the
> problem comes to us from the RC16 code which unconditionally sets WS16).
>
> James
James,
This patch works for me; are you planning on committing this patch.
Regards,
K. Y
>
> ---
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 6825eda..8353a4c
> 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -634,6 +634,23 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> unsigned int mode)
> max(sdkp->physical_block_size,
> sdkp->unmap_granularity * logical_block_size);
>
> + if (sdkp->device->host->no_write_same) {
> + switch(mode) {
> +
> + case SD_LBP_WS16:
> + case SD_LBP_WS10:
> + case SD_LBP_ZERO:
> + /*
> + * filter out all the WRITE SAME modes and map them
> + * directly to UNMAP
> + */
> + mode = SD_LBP_UNMAP;
> + /* fall through */
> + default:
> + /* everything else is OK */
> + break;
> + }
> + }
> sdkp->provisioning_mode = mode;
>
> switch (mode) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 09, 2014 at 10:36:26PM +, KY Srinivasan wrote: > Ok; I am concerned about older kernels that do not have no_write_same flag. > I suppose I can work directly with these Distros and give them a choice: > either implement > the no_write_same flag or filter the command in our driver. I will not > include this patch when Exactly. In general they should be interested in the flag as various raid controllers react badly to it as well. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, Jul 09, 2014 at 10:36:26PM +, KY Srinivasan wrote: Ok; I am concerned about older kernels that do not have no_write_same flag. I suppose I can work directly with these Distros and give them a choice: either implement the no_write_same flag or filter the command in our driver. I will not include this patch when Exactly. In general they should be interested in the flag as various raid controllers react badly to it as well. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message-
From: James Bottomley [mailto:jbottom...@parallels.com]
Sent: Wednesday, July 9, 2014 3:27 PM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org;
de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote:
-Original Message-
From: James Bottomley [mailto:jbottom...@parallels.com]
Sent: Wednesday, July 9, 2014 12:57 PM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; h...@infradead.org;
de...@linuxdriverproject.org; a...@canonical.com;
sta...@vger.kernel.org; linux-s...@vger.kernel.org;
oher...@suse.com; jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
WRITE_SAME_16
On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Wednesday, July 9, 2014 1:43 AM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
oher...@suse.com; jbottom...@parallels.com;
jasow...@redhat.com;
a...@canonical.com; linux-s...@vger.kernel.org;
sta...@vger.kernel.org
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
WRITE_SAME_16
On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote:
Host does not handle WRITE_SAME_16; filter this command out.
This patch is required to handle large devices (greater than 2 TB
disks).
Storvsc already sets the no_write_same flag, where is the
command coming from?
In spite of this flag, I see WRITE_SAME_16 being issued when I
format a device bigger than 2 TB; I tried both xfs and ext4.
Windows hosts currently do not handle unsupported commands
correctly - The information returned is not sufficient to effect
recovery
in the Linux guest.
While this may be addressed in future hosts, this patch fixes the problem.
What Christoph means is that this looks like a bug somewhere in SCSI
itself.
That means we need to find it and kill it, not add workarounds to
every driver that sets no_write_same ...
James,
I will try to isolate this issue in the SCSI stack. If it is ok with
you guys, I would still want to filter WRITE_SAME_16 (as we currently
do WRITE_SAME) in our driver since this would address the problem for a
large number of customers on our platform.
If we fix it at source, why would there be any need to filter? That's the
reason the no_write_same flag was introduced. If we can find and fix the
bug, it can go back into the stable trees as a bug fix, hence nothing should
ever emit write_same(10 or 16) and additional driver code is redundant (and
counter productive, since if this ever breaks again you're our best canary).
This looks like it might be the problem but Martin should confirm (I think the
problem comes to us from the RC16 code which unconditionally sets WS16).
James
James,
This patch works for me; are you planning on committing this patch.
Regards,
K. Y
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 6825eda..8353a4c
100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -634,6 +634,23 @@ static void sd_config_discard(struct scsi_disk *sdkp,
unsigned int mode)
max(sdkp-physical_block_size,
sdkp-unmap_granularity * logical_block_size);
+ if (sdkp-device-host-no_write_same) {
+ switch(mode) {
+
+ case SD_LBP_WS16:
+ case SD_LBP_WS10:
+ case SD_LBP_ZERO:
+ /*
+ * filter out all the WRITE SAME modes and map them
+ * directly to UNMAP
+ */
+ mode = SD_LBP_UNMAP;
+ /* fall through */
+ default:
+ /* everything else is OK */
+ break;
+ }
+ }
sdkp-provisioning_mode = mode;
switch (mode) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Thu, 2014-07-10 at 21:02 +, KY Srinivasan wrote: -Original Message- From: James Bottomley [mailto:jbottom...@parallels.com] Sent: Wednesday, July 9, 2014 3:27 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote: -Original Message- From: James Bottomley [mailto:jbottom...@parallels.com] Sent: Wednesday, July 9, 2014 12:57 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, July 9, 2014 1:43 AM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James, I will try to isolate this issue in the SCSI stack. If it is ok with you guys, I would still want to filter WRITE_SAME_16 (as we currently do WRITE_SAME) in our driver since this would address the problem for a large number of customers on our platform. If we fix it at source, why would there be any need to filter? That's the reason the no_write_same flag was introduced. If we can find and fix the bug, it can go back into the stable trees as a bug fix, hence nothing should ever emit write_same(10 or 16) and additional driver code is redundant (and counter productive, since if this ever breaks again you're our best canary). This looks like it might be the problem but Martin should confirm (I think the problem comes to us from the RC16 code which unconditionally sets WS16). James James, This patch works for me; are you planning on committing this patch. OK, if we go with this we can add your tested by. It's not going in until Martin takes a look because there may be a better way of doing this. James
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message- > From: James Bottomley [mailto:jbottom...@parallels.com] > Sent: Wednesday, July 9, 2014 3:27 PM > To: KY Srinivasan > Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org; > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote: > > > > > -Original Message- > > > From: James Bottomley [mailto:jbottom...@parallels.com] > > > Sent: Wednesday, July 9, 2014 12:57 PM > > > To: KY Srinivasan > > > Cc: linux-kernel@vger.kernel.org; h...@infradead.org; > > > de...@linuxdriverproject.org; a...@canonical.com; > > > sta...@vger.kernel.org; linux-s...@vger.kernel.org; > > > oher...@suse.com; jasow...@redhat.com > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > WRITE_SAME_16 > > > > > > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > > > > > > > -Original Message- > > > > > From: Christoph Hellwig [mailto:h...@infradead.org] > > > > > Sent: Wednesday, July 9, 2014 1:43 AM > > > > > To: KY Srinivasan > > > > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > > > > > oher...@suse.com; jbottom...@parallels.com; > jasow...@redhat.com; > > > > > a...@canonical.com; linux-s...@vger.kernel.org; > > > > > sta...@vger.kernel.org > > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > > > WRITE_SAME_16 > > > > > > > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > > > > Host does not handle WRITE_SAME_16; filter this command out. > > > > > > This patch is required to handle large devices (greater than 2 TB > disks). > > > > > > > > > > Storvsc already sets the no_write_same flag, where is the > > > > > command coming from? > > > > > > > > In spite of this flag, I see WRITE_SAME_16 being issued when I > > > > format a device bigger than 2 TB; I tried both xfs and ext4. > > > > Windows hosts currently do not handle unsupported commands > > > > correctly - The information returned is not sufficient to effect > > > > recovery > in the Linux guest. > > > While this may be addressed in future hosts, this patch fixes the problem. > > > > > > What Christoph means is that this looks like a bug somewhere in SCSI > itself. > > > That means we need to find it and kill it, not add workarounds to > > > every driver that sets no_write_same ... > > > > James, > > > > I will try to isolate this issue in the SCSI stack. If it is ok with > > you guys, I would still want to filter WRITE_SAME_16 (as we currently > > do WRITE_SAME) in our driver since this would address the problem for a > large number of customers on our platform. > > If we fix it at source, why would there be any need to filter? That's the > reason the no_write_same flag was introduced. If we can find and fix the > bug, it can go back into the stable trees as a bug fix, hence nothing should > ever emit write_same(10 or 16) and additional driver code is redundant (and > counter productive, since if this ever breaks again you're our best canary). > > This looks like it might be the problem but Martin should confirm (I think the > problem comes to us from the RC16 code which unconditionally sets WS16). Ok; I am concerned about older kernels that do not have no_write_same flag. I suppose I can work directly with these Distros and give them a choice: either implement the no_write_same flag or filter the command in our driver. I will not include this patch when I resubmit this patch-set. Thanks, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:jbottom...@parallels.com]
> > Sent: Wednesday, July 9, 2014 12:57 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; h...@infradead.org;
> > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
> > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> >
> > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote:
> > >
> > > > -Original Message-
> > > > From: Christoph Hellwig [mailto:h...@infradead.org]
> > > > Sent: Wednesday, July 9, 2014 1:43 AM
> > > > To: KY Srinivasan
> > > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > > > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com;
> > > > a...@canonical.com; linux-s...@vger.kernel.org;
> > > > sta...@vger.kernel.org
> > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
> > > > WRITE_SAME_16
> > > >
> > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote:
> > > > > Host does not handle WRITE_SAME_16; filter this command out. This
> > > > > patch is required to handle large devices (greater than 2 TB disks).
> > > >
> > > > Storvsc already sets the no_write_same flag, where is the command
> > > > coming from?
> > >
> > > In spite of this flag, I see WRITE_SAME_16 being issued when I format
> > > a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts
> > > currently do not handle unsupported commands correctly - The
> > > information returned is not sufficient to effect recovery in the Linux
> > > guest.
> > While this may be addressed in future hosts, this patch fixes the problem.
> >
> > What Christoph means is that this looks like a bug somewhere in SCSI itself.
> > That means we need to find it and kill it, not add workarounds to every
> > driver
> > that sets no_write_same ...
>
> James,
>
> I will try to isolate this issue in the SCSI stack. If it is ok with you
> guys, I would still want to filter WRITE_SAME_16
> (as we currently do WRITE_SAME) in our driver since this would address the
> problem for a large number of
> customers on our platform.
If we fix it at source, why would there be any need to filter? That's
the reason the no_write_same flag was introduced. If we can find and
fix the bug, it can go back into the stable trees as a bug fix, hence
nothing should ever emit write_same(10 or 16) and additional driver code
is redundant (and counter productive, since if this ever breaks again
you're our best canary).
This looks like it might be the problem but Martin should confirm (I
think the problem comes to us from the RC16 code which unconditionally
sets WS16).
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6825eda..8353a4c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -634,6 +634,23 @@ static void sd_config_discard(struct scsi_disk *sdkp,
unsigned int mode)
max(sdkp->physical_block_size,
sdkp->unmap_granularity * logical_block_size);
+ if (sdkp->device->host->no_write_same) {
+ switch(mode) {
+
+ case SD_LBP_WS16:
+ case SD_LBP_WS10:
+ case SD_LBP_ZERO:
+ /*
+* filter out all the WRITE SAME modes and map them
+* directly to UNMAP
+*/
+ mode = SD_LBP_UNMAP;
+ /* fall through */
+ default:
+ /* everything else is OK */
+ break;
+ }
+ }
sdkp->provisioning_mode = mode;
switch (mode) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message- > From: James Bottomley [mailto:jbottom...@parallels.com] > Sent: Wednesday, July 9, 2014 12:57 PM > To: KY Srinivasan > Cc: linux-kernel@vger.kernel.org; h...@infradead.org; > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > > > -Original Message- > > > From: Christoph Hellwig [mailto:h...@infradead.org] > > > Sent: Wednesday, July 9, 2014 1:43 AM > > > To: KY Srinivasan > > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > > > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; > > > a...@canonical.com; linux-s...@vger.kernel.org; > > > sta...@vger.kernel.org > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > WRITE_SAME_16 > > > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > > Host does not handle WRITE_SAME_16; filter this command out. This > > > > patch is required to handle large devices (greater than 2 TB disks). > > > > > > Storvsc already sets the no_write_same flag, where is the command > > > coming from? > > > > In spite of this flag, I see WRITE_SAME_16 being issued when I format > > a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts > > currently do not handle unsupported commands correctly - The > > information returned is not sufficient to effect recovery in the Linux > > guest. > While this may be addressed in future hosts, this patch fixes the problem. > > What Christoph means is that this looks like a bug somewhere in SCSI itself. > That means we need to find it and kill it, not add workarounds to every driver > that sets no_write_same ... James, I will try to isolate this issue in the SCSI stack. If it is ok with you guys, I would still want to filter WRITE_SAME_16 (as we currently do WRITE_SAME) in our driver since this would address the problem for a large number of customers on our platform. Regards, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > -Original Message- > > From: Christoph Hellwig [mailto:h...@infradead.org] > > Sent: Wednesday, July 9, 2014 1:43 AM > > To: KY Srinivasan > > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; > > a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > Host does not handle WRITE_SAME_16; filter this command out. This > > > patch is required to handle large devices (greater than 2 TB disks). > > > > Storvsc already sets the no_write_same flag, where is the command coming > > from? > > In spite of this flag, I see WRITE_SAME_16 being issued when I format a > device bigger than 2 TB; > I tried both xfs and ext4. Windows hosts currently do not handle unsupported > commands correctly - > The information returned is not sufficient to effect recovery in the Linux > guest. While this may be addressed in > future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> -Original Message- > From: Christoph Hellwig [mailto:h...@infradead.org] > Sent: Wednesday, July 9, 2014 1:43 AM > To: KY Srinivasan > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; > a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > Host does not handle WRITE_SAME_16; filter this command out. This > > patch is required to handle large devices (greater than 2 TB disks). > > Storvsc already sets the no_write_same flag, where is the command coming > from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > Host does not handle WRITE_SAME_16; filter this command out. This patch > is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, July 9, 2014 1:43 AM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, July 9, 2014 1:43 AM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message- From: James Bottomley [mailto:jbottom...@parallels.com] Sent: Wednesday, July 9, 2014 12:57 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, July 9, 2014 1:43 AM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James, I will try to isolate this issue in the SCSI stack. If it is ok with you guys, I would still want to filter WRITE_SAME_16 (as we currently do WRITE_SAME) in our driver since this would address the problem for a large number of customers on our platform. Regards, K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote:
-Original Message-
From: James Bottomley [mailto:jbottom...@parallels.com]
Sent: Wednesday, July 9, 2014 12:57 PM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; h...@infradead.org;
de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Wednesday, July 9, 2014 1:43 AM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com;
a...@canonical.com; linux-s...@vger.kernel.org;
sta...@vger.kernel.org
Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
WRITE_SAME_16
On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote:
Host does not handle WRITE_SAME_16; filter this command out. This
patch is required to handle large devices (greater than 2 TB disks).
Storvsc already sets the no_write_same flag, where is the command
coming from?
In spite of this flag, I see WRITE_SAME_16 being issued when I format
a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts
currently do not handle unsupported commands correctly - The
information returned is not sufficient to effect recovery in the Linux
guest.
While this may be addressed in future hosts, this patch fixes the problem.
What Christoph means is that this looks like a bug somewhere in SCSI itself.
That means we need to find it and kill it, not add workarounds to every
driver
that sets no_write_same ...
James,
I will try to isolate this issue in the SCSI stack. If it is ok with you
guys, I would still want to filter WRITE_SAME_16
(as we currently do WRITE_SAME) in our driver since this would address the
problem for a large number of
customers on our platform.
If we fix it at source, why would there be any need to filter? That's
the reason the no_write_same flag was introduced. If we can find and
fix the bug, it can go back into the stable trees as a bug fix, hence
nothing should ever emit write_same(10 or 16) and additional driver code
is redundant (and counter productive, since if this ever breaks again
you're our best canary).
This looks like it might be the problem but Martin should confirm (I
think the problem comes to us from the RC16 code which unconditionally
sets WS16).
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6825eda..8353a4c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -634,6 +634,23 @@ static void sd_config_discard(struct scsi_disk *sdkp,
unsigned int mode)
max(sdkp-physical_block_size,
sdkp-unmap_granularity * logical_block_size);
+ if (sdkp-device-host-no_write_same) {
+ switch(mode) {
+
+ case SD_LBP_WS16:
+ case SD_LBP_WS10:
+ case SD_LBP_ZERO:
+ /*
+* filter out all the WRITE SAME modes and map them
+* directly to UNMAP
+*/
+ mode = SD_LBP_UNMAP;
+ /* fall through */
+ default:
+ /* everything else is OK */
+ break;
+ }
+ }
sdkp-provisioning_mode = mode;
switch (mode) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
-Original Message- From: James Bottomley [mailto:jbottom...@parallels.com] Sent: Wednesday, July 9, 2014 3:27 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; m...@mkp.net; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote: -Original Message- From: James Bottomley [mailto:jbottom...@parallels.com] Sent: Wednesday, July 9, 2014 12:57 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; h...@infradead.org; de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, July 9, 2014 1:43 AM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: Host does not handle WRITE_SAME_16; filter this command out. This patch is required to handle large devices (greater than 2 TB disks). Storvsc already sets the no_write_same flag, where is the command coming from? In spite of this flag, I see WRITE_SAME_16 being issued when I format a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts currently do not handle unsupported commands correctly - The information returned is not sufficient to effect recovery in the Linux guest. While this may be addressed in future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James, I will try to isolate this issue in the SCSI stack. If it is ok with you guys, I would still want to filter WRITE_SAME_16 (as we currently do WRITE_SAME) in our driver since this would address the problem for a large number of customers on our platform. If we fix it at source, why would there be any need to filter? That's the reason the no_write_same flag was introduced. If we can find and fix the bug, it can go back into the stable trees as a bug fix, hence nothing should ever emit write_same(10 or 16) and additional driver code is redundant (and counter productive, since if this ever breaks again you're our best canary). This looks like it might be the problem but Martin should confirm (I think the problem comes to us from the RC16 code which unconditionally sets WS16). Ok; I am concerned about older kernels that do not have no_write_same flag. I suppose I can work directly with these Distros and give them a choice: either implement the no_write_same flag or filter the command in our driver. I will not include this patch when I resubmit this patch-set. Thanks, K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
