From: Nicolai Stange <nicstange@gmail.com>
To: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Nicolai Stange <nicstange@gmail.com>,
Jens Axboe <axboe@kernel.dk>,
"James E.J. Bottomley" <jejb@linux.vnet.ibm.com>,
Shaun Tancheff <shaun.tancheff@seagate.com>,
Chaitanya Kulkarni <chaitanya.kulkarni@hgst.com>,
Christoph Hellwig <hch@infradead.org>,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sd: make ->no_write_same independent of reported ->max_ws_blocks
Date: Tue, 06 Dec 2016 09:08:19 +0100 [thread overview]
Message-ID: <87lgvteaz0.fsf@gmail.com> (raw)
In-Reply-To: <yq1d1h5viob.fsf@sermon.lab.mkp.net> (Martin K. Petersen's message of "Mon, 05 Dec 2016 22:29:56 -0500")
Hello Martin,
"Martin K. Petersen" <martin.petersen@oracle.com> writes:
>>>>>> "Nicolai" == Nicolai Stange <nicstange@gmail.com> writes:
> Nicolai> Due to reported problems with Write Same on ATA devices, commit
> Nicolai> 0ce1b18c42a5 ("libata: Some drives failing on SCT Write Same")
> Nicolai> strived to report non-support for Write Same on non-zoned ATA
> Nicolai> devices.
>
> Nicolai> However, due to the following control flow in
> Nicolai> sd_config_write_same() this doesn't always take effect, namely
> Nicolai> if the ->max_ws_blocks as set in the by the ATA Identify Device
> Nicolai> exceeds SD_WS10_BLOCKS:
>
> I'd much prefer for libata to set no_write_same = 1 for non-ZAC devices.
Or just try it once and let the sd layer, i.e. sd_done(), disable it
once a ILLEGAL COMMAND OPCODE is reported. This works right now and as
you said below, calling code must cope gracefully with a failing Write
Same anyway (which doesn't work right now).
>
> Older SCSI devices have no way to explicitly report that WRITE SAME is
> supported. So the heuristic is the way it is to permit trying WRITE SAME
> unless no_write_same has been set by the device driver.
Ok, I didn't see that there might be a heuristic going on.
I've got a couple of questions about this, but they're mainly out of
curiosity. So feel free to ignore them.
1.) Do these older SCSI devices have a way to report ->max_ws_blocks?
Because otherwise the heuristic would not work?
Or is it set speculatively somewhere?
2.) If so, what about such older devices having
0 < ->max_ws_blocks < SD_MAX_WS10_BLOCKS ?
Wouldn't these also be suitable candidates for trying that
heuristic on?
3.) Those older devices that have ->max_ws_blocks > SD_MAX_WS10_BLOCKS
but ->ws16 == ->ws10 == 0, i.e. the heuristicated ones would
always be given WRITE_SAME, not WRITE_SAME_16 commands?
C.f. sd_setup_write_same_cmnd(): if ->ws16 is not set, do
WRITE_SAME. Isn't this a little bit odd given that the reported
->max_ws_blocks would be greater than SD_MAX_WS10_BLOCKS?
Ok, given that these devices are older anyway, WRITE_SAME seems
like the obvious choice to be made over WRITE_SAME_16. Which
brings me back to question 2.).
The answer to this question would possibly affect ATA devices with
this heuristic going on as well: according to ata_scsiop_maint_in(),
they would only support WRITE_SAME_16, but not WRITE_SAME.
Heck, this is perhaps the reason why I'm seeing those errors this
commit 0ce1b18c42a5 ("libata: Some drives failing on SCT Write
Same") effectively turns the heuristics for my ATA device on,
i.e. unsets ->ws16, resulting in WRITE_SAME's which are unsupported
by libata-scsi, c.f. ata_get_xlat_func()...
>
> Nicolai> Since commit e73c23ff736e ("block: add async variant of
> Nicolai> blkdev_issue_zeroout"), blkdev_issue_zeroout() got a little bit
> Nicolai> more sensitive towards failing Write Sames on devices that
> Nicolai> claim to support them and this results in messages like
>
> That's something that needs to be addressed. blkdev_issue_zeroout() must
> cope with WRITE SAME failing and fall back to a manual zeroout.
That's very useful information! So this commit really needs a fixup in
either way.
Thank you!
Nicolai
next prev parent reply other threads:[~2016-12-06 8:08 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-05 23:56 [PATCH] sd: make ->no_write_same independent of reported ->max_ws_blocks Nicolai Stange
2016-12-06 3:29 ` Martin K. Petersen
2016-12-06 8:08 ` Nicolai Stange [this message]
2016-12-08 0:18 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lgvteaz0.fsf@gmail.com \
--to=nicstange@gmail.com \
--cc=axboe@kernel.dk \
--cc=chaitanya.kulkarni@hgst.com \
--cc=hch@infradead.org \
--cc=jejb@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=shaun.tancheff@seagate.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).