All of lore.kernel.org
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Paolo Valente <paolo.valente@linaro.org>
Cc: Hannes Reinecke <hare@suse.de>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	"linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>
Subject: Re: [PATCH v3 1/4] block: Add concurrent positioning ranges support
Date: Tue, 27 Jul 2021 23:44:54 +0000	[thread overview]
Message-ID: <DM6PR04MB7081D6168AF0E59D4EC33011E7E99@DM6PR04MB7081.namprd04.prod.outlook.com> (raw)
In-Reply-To: F59D5B88-5CEF-4A61-A8AC-9FF572A462DC@linaro.org

On 2021/07/27 23:07, Paolo Valente wrote:
> 
> 
>> Il giorno 26 lug 2021, alle ore 13:33, Damien Le Moal <Damien.LeMoal@wdc.com> ha scritto:
>>
>> On 2021/07/26 17:47, Hannes Reinecke wrote:
>>> On 7/26/21 10:30 AM, Damien Le Moal wrote:
>>>> On 2021/07/26 16:34, Hannes Reinecke wrote:
>>> [ .. ]
>>>>> In principle it looks good, but what would be the appropriate action
>>>>> when invalid ranges are being detected during revalidation?
>>>>> The current code will leave the original ones intact, but I guess that's
>>>>> questionable as the current settings are most likely invalid.
>>>>
>>>> Nope. In that case, the old ranges are removed. In blk_queue_set_cranges(),
>>>> there is:
>>>>
>>>> +		if (!blk_check_ranges(disk, cr)) {
>>>> +			kfree(cr);
>>>> +			cr = NULL;
>>>> +			goto reg;
>>>> +		}
>>>>
>>>> So for incorrect ranges, we will register "NULL", so no ranges. The old ranges
>>>> are gone.
>>>>
>>>
>>> Right. So that's the first concern addressed.
>>
>> Not that at the scsi layer, if there is an error retrieving the ranges
>> informations, blk_queue_set_cranges(q, NULL) is called, so the same happen: the
>> ranges set are removed and no range information will appear in sysfs.
>>
> 
> As a very personal opinion, silent failures are often misleading when
> trying to understand what is going wrong in a system.  But I guess
> this is however the best option.

Failure are not silent: error messages are printed and will be visible in dmesg.

We can always completely ignore the drive and completely fail its initialization
in the case of a failed cranges initialization. But I find that rather extreme
since the drive is supposed to work anyway, even without any access optimization
for the multiple cranges case.

> 
> Thanks,
> Paolo
> 
>>>
>>>>> I would vote for de-register the old ones and implement an error state
>>>>> (using an error pointer?); that would signal that there _are_ ranges,
>>>>> but we couldn't parse them properly.
>>>>> Hmm?
>>>>
>>>> With the current code, the information "there are ranges" will be completely
>>>> gone if the ranges are bad... dmesg will have a message about it, but that's it.
>>>>
>>> So there will be no additional information in sysfs in case of incorrect 
>>> ranges?
>>
>> Yep, there will be no queue/cranges directory. The drive will be the same as a
>> single actuator one.
>>
>>> Hmm. Not sure if I like that, but then it might be the best option after 
>>> all. So you can add my:
>>
>> Nothing much that we can do. If we fail to retrieve the ranges, or the ranges
>> are incorrect, access optimization by FS or scheduler is not really possible.
>> Note that the drive will still work. Only any eventual optimization will be
>> turned off.
>>
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>
>> Thanks !
>>
>>>
>>> Cheers,
>>>
>>> Hannes
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
> 
> 


-- 
Damien Le Moal
Western Digital Research

  reply	other threads:[~2021-07-27 23:44 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26  1:38 [PATCH v3 0/4] Initial support for multi-actuator HDDs Damien Le Moal
2021-07-26  1:38 ` [PATCH v3 1/4] block: Add concurrent positioning ranges support Damien Le Moal
2021-07-26  7:33   ` Hannes Reinecke
2021-07-26  8:30     ` Damien Le Moal
2021-07-26  8:47       ` Hannes Reinecke
2021-07-26 11:33         ` Damien Le Moal
2021-07-27 14:07           ` Paolo Valente
2021-07-27 23:44             ` Damien Le Moal [this message]
2021-08-10  8:23   ` Christoph Hellwig
2021-08-10 11:03     ` Damien Le Moal
2021-08-10 16:02       ` hch
2021-08-10 23:46         ` Damien Le Moal
2021-07-26  1:38 ` [PATCH v3 2/4] scsi: sd: add " Damien Le Moal
2021-08-10  8:24   ` Christoph Hellwig
2021-07-26  1:38 ` [PATCH v3 3/4] libata: support concurrent positioning ranges log Damien Le Moal
2021-07-26  7:34   ` Hannes Reinecke
2021-08-10  8:26   ` Christoph Hellwig
2021-07-26  1:38 ` [PATCH v3 4/4] doc: document sysfs queue/cranges attributes Damien Le Moal
2021-07-26  7:35   ` Hannes Reinecke
2021-08-10  8:27   ` Christoph Hellwig
2021-08-10 11:04     ` Damien Le Moal
2021-07-28 22:59 ` [PATCH v3 0/4] Initial support for multi-actuator HDDs Damien Le Moal
2021-08-06  2:12 ` Damien Le Moal
2021-08-06  3:41 ` Martin K. Petersen
2021-08-06  4:05   ` Damien Le Moal
2021-08-06  8:35     ` Hannes Reinecke
2021-08-06  8:52       ` Damien Le Moal

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=DM6PR04MB7081D6168AF0E59D4EC33011E7E99@DM6PR04MB7081.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=hare@suse.de \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=paolo.valente@linaro.org \
    /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.