Re: [PATCH 0/4] Drivers: scsi: storvsc: Fix miscellaneous issues
On Mon, Dec 29, 2014 at 09:07:59PM +, KY Srinivasan wrote: > Should I be resending these patches. I don't need a resend, I need a review for the patches. Note that for driver patches I'm also fine with a review from a co worker, as long as it's a real review not just a rubber stamp. Talking about process: for the next submission please only use "storvsc: " as the subject prefix, I had to remove "Drivers: scsi: " which doesn'd add useful information every time so far. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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.. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, Jul 18, 2014 at 04:57:13PM +, James Bottomley wrote: > Actually, no you didn't. The difference is in the derivation of the > timeout. Christoph's patch is absolute in terms of SD_TIMEOUT; yours is > relative to the queue timeout setting ... I thought there was a reason > for preferring the relative version. Yes, KYs version is better. It takes the base timeout drivers set on the request queue into account. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
This is what I plan to put in after it passes basic testing: --- >From bb617c9465b839d70ecbbc69002a20ccf5f935bd Mon Sep 17 00:00:00 2001 From: "K. Y. Srinivasan" Date: Fri, 18 Jul 2014 19:12:58 +0200 Subject: sd: fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this patch did not use the basic I/O timeout of the device. Fix this bug. Signed-off-by: K. Y. Srinivasan Reviewed-by: James Bottomley Signed-off-by: Christoph Hellwig --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index bef4e78..9ffb393 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -880,7 +880,7 @@ static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd) cmd->transfersize = 0; cmd->allowed = SD_MAX_RETRIES; - rq->timeout = SD_TIMEOUT * SD_FLUSH_TIMEOUT_MULTIPLIER; + rq->timeout = rq->q->rq_timeout * SD_FLUSH_TIMEOUT_MULTIPLIER; return BLKPREP_OK; } -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, Jul 18, 2014 at 10:12:38AM -0700, h...@infradead.org wrote: > This is what I plan to put in after it passes basic testing: And that one was on top of my previous version. One that applies against core-for-3.17 below: --- >From 8a79783e5f72ec034a724e16c1f46604bd97bf68 Mon Sep 17 00:00:00 2001 From: "K. Y. Srinivasan" Date: Fri, 18 Jul 2014 17:11:27 +0200 Subject: sd: fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this patch did not use the basic I/O timeout of the device. Fix this bug. Signed-off-by: K. Y. Srinivasan Reviewed-by: James Bottomley Signed-off-by: Christoph Hellwig --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 377a520..9ffb393 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -880,7 +880,7 @@ static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd) cmd->transfersize = 0; cmd->allowed = SD_MAX_RETRIES; - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER; + rq->timeout = rq->q->rq_timeout * SD_FLUSH_TIMEOUT_MULTIPLIER; return BLKPREP_OK; } -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Thu, Jul 17, 2014 at 11:53:33PM +, KY Srinivasan wrote: > I still see this problem. There was talk of fixing it elsewhere. Well, what we have right not is entirely broken, given that the block layer doesn't initialize ->timeout on TYPE_FS requeuests. We either need to revert that initial commit or apply something like the attached patch as a quick fix. >From ecbf154d15f4022676219b9ff90e542d1db64392 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 18 Jul 2014 17:11:27 +0200 Subject: sd: set a non-zero timeout for flush requests rq->timeout for TYPE_FS commands needs to be initialized by the driver, so we can't simply multiply it. Signed-off-by: Christoph Hellwig --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 377a520..bef4e78 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -880,7 +880,7 @@ static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd) cmd->transfersize = 0; cmd->allowed = SD_MAX_RETRIES; - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER; + rq->timeout = SD_TIMEOUT * SD_FLUSH_TIMEOUT_MULTIPLIER; return BLKPREP_OK; } -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, Jul 18, 2014 at 12:51:06AM +, Elliott, Robert (Server Storage) wrote: > SYNCHRONIZE CACHE (16) should be favored over SYNCHRONIZE > CACHE (10) unless SYNCHRONIZE CACHE (10) is not supported. I gues you mean (16) for the last occurance? What's the benefit of using SYNCHRONIZE CACHE (16) if we don't pass a LBA range? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
