From: "Martin K. Petersen" <martin.petersen@oracle.com>
To: Damien Le Moal <damien.lemoal@hgst.com>
Cc: Jens Axboe <axboe@kernel.dk>, <linux-block@vger.kernel.org>,
<linux-scsi@vger.kernel.org>, Christoph Hellwig <hch@lst.de>,
"Martin K . Petersen" <martin.petersen@oracle.com>,
Hannes Reinecke <hare@suse.de>,
Shaun Tancheff <shaun@tancheff.com>
Subject: Re: [PATCH v3 6/7] sd: Implement support for ZBC devices
Date: Wed, 28 Sep 2016 01:01:04 -0400 [thread overview]
Message-ID: <yq18tuczkhb.fsf@sermon.lab.mkp.net> (raw)
In-Reply-To: <1475025757-6340-7-git-send-email-damien.lemoal@hgst.com> (Damien Le Moal's message of "Wed, 28 Sep 2016 10:22:36 +0900")
>>>>> "Damien" == Damien Le Moal <damien.lemoal@hgst.com> writes:
Damien,
The new stuff looks much cleaner. Thanks for doing that!
This hunk has an unintended side effect:
@@ -2836,14 +2896,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
* react badly if we do.
*/
if (sdkp->media_present) {
- sd_read_capacity(sdkp, buffer);
-
if (scsi_device_supports_vpd(sdp)) {
sd_read_block_provisioning(sdkp);
sd_read_block_limits(sdkp);
sd_read_block_characteristics(sdkp);
}
+ sd_read_capacity(sdkp, buffer);
+
sd_read_write_protect_flag(sdkp, buffer);
sd_read_cache_type(sdkp, buffer);
sd_read_app_tag_own(sdkp, buffer);
LMBPE from READ CAPACITY(16) will not be set when calling
sd_read_block_provisioning() and thus we bail early (we catch it second
time around). You may want to split that vpd conditional to shuffle the
block_characteristics on top. Or even better: Split the capacity/block
size printing code from sd_read_capacity() into a separate function and
do:
if (sdkp->media_present) {
sd_read_capacity(sdkp, buffer);
if (sd_try_extended_inquiry(sdp)) {
sd_read_block_provisioning(sdkp);
sd_read_block_limits(sdkp);
sd_read_block_characteristics(sdkp);
}
+ sd_print_capacity(sdkp);
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);
}
Typo:
static int sd_zbc_read_zoned_charateristics(struct scsi_disk *sdkp,
^^^^^^^^^^^^^^
My second comment is that I don't particularly like the notion of values
being stored and passed in units of block layer sectors in struct
scsi_disk and sd*. As a rule of thumb I prefer SCSI stuff to be counted
in logical blocks and block layer stuff in 512b sectors. You may want to
entertain having a sectors_to_zone() wrapper to facilitate that
conversion.
--
Martin K. Petersen Oracle Linux Engineering
WARNING: multiple messages have this Message-ID (diff)
From: "Martin K. Petersen" <martin.petersen@oracle.com>
To: Damien Le Moal <damien.lemoal@hgst.com>
Cc: Jens Axboe <axboe@kernel.dk>,
linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
Christoph Hellwig <hch@lst.de>,
"Martin K . Petersen" <martin.petersen@oracle.com>,
Hannes Reinecke <hare@suse.de>,
Shaun Tancheff <shaun@tancheff.com>
Subject: Re: [PATCH v3 6/7] sd: Implement support for ZBC devices
Date: Wed, 28 Sep 2016 01:01:04 -0400 [thread overview]
Message-ID: <yq18tuczkhb.fsf@sermon.lab.mkp.net> (raw)
In-Reply-To: <1475025757-6340-7-git-send-email-damien.lemoal@hgst.com> (Damien Le Moal's message of "Wed, 28 Sep 2016 10:22:36 +0900")
>>>>> "Damien" == Damien Le Moal <damien.lemoal@hgst.com> writes:
Damien,
The new stuff looks much cleaner. Thanks for doing that!
This hunk has an unintended side effect:
@@ -2836,14 +2896,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
* react badly if we do.
*/
if (sdkp->media_present) {
- sd_read_capacity(sdkp, buffer);
-
if (scsi_device_supports_vpd(sdp)) {
sd_read_block_provisioning(sdkp);
sd_read_block_limits(sdkp);
sd_read_block_characteristics(sdkp);
}
+ sd_read_capacity(sdkp, buffer);
+
sd_read_write_protect_flag(sdkp, buffer);
sd_read_cache_type(sdkp, buffer);
sd_read_app_tag_own(sdkp, buffer);
LMBPE from READ CAPACITY(16) will not be set when calling
sd_read_block_provisioning() and thus we bail early (we catch it second
time around). You may want to split that vpd conditional to shuffle the
block_characteristics on top. Or even better: Split the capacity/block
size printing code from sd_read_capacity() into a separate function and
do:
if (sdkp->media_present) {
sd_read_capacity(sdkp, buffer);
if (sd_try_extended_inquiry(sdp)) {
sd_read_block_provisioning(sdkp);
sd_read_block_limits(sdkp);
sd_read_block_characteristics(sdkp);
}
+ sd_print_capacity(sdkp);
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);
}
Typo:
static int sd_zbc_read_zoned_charateristics(struct scsi_disk *sdkp,
^^^^^^^^^^^^^^
My second comment is that I don't particularly like the notion of values
being stored and passed in units of block layer sectors in struct
scsi_disk and sd*. As a rule of thumb I prefer SCSI stuff to be counted
in logical blocks and block layer stuff in 512b sectors. You may want to
entertain having a sectors_to_zone() wrapper to facilitate that
conversion.
--
Martin K. Petersen Oracle Linux Engineering
next prev parent reply other threads:[~2016-09-28 5:01 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-28 1:22 [PATCH v3 0/7] ZBC / Zoned block device support Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 1:22 ` [PATCH v3 1/7] block: Add 'zoned' queue limit Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 3:42 ` Martin K. Petersen
2016-09-28 3:42 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 2/7] blk-sysfs: Add 'chunk_sectors' to sysfs attributes Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 3:43 ` Martin K. Petersen
2016-09-28 3:43 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 3/7] block: update chunk_sectors in blk_stack_limits() Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 3:44 ` Martin K. Petersen
2016-09-28 3:44 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 4/7] block: Define zoned block device operations Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 3:45 ` Martin K. Petersen
2016-09-28 3:45 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 5/7] block: Implement support for zoned block devices Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 3:54 ` Martin K. Petersen
2016-09-28 3:54 ` Martin K. Petersen
2016-09-28 4:21 ` Damien Le Moal
2016-09-28 4:21 ` Damien Le Moal
2016-09-28 5:05 ` Martin K. Petersen
2016-09-28 5:05 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 6/7] sd: Implement support for ZBC devices Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 5:01 ` Martin K. Petersen [this message]
2016-09-28 5:01 ` Martin K. Petersen
2016-09-28 1:22 ` [PATCH v3 7/7] blk-zoned: implement ioctls Damien Le Moal
2016-09-28 1:22 ` Damien Le Moal
2016-09-28 5:03 ` Martin K. Petersen
2016-09-28 5:03 ` Martin K. Petersen
2016-09-28 2:04 ` [PATCH v3 0/7] ZBC / Zoned block device support Christoph Hellwig
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=yq18tuczkhb.fsf@sermon.lab.mkp.net \
--to=martin.petersen@oracle.com \
--cc=axboe@kernel.dk \
--cc=damien.lemoal@hgst.com \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=shaun@tancheff.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.