[dm-devel] dm-raid: stack limits instead of overwriting them.

[S. Baerwolf]

Hi

I just saw your posts right now. (After posting my text "[PATCH] dm-raid: set discard_granularity non-zero if possible" yesterday)

How about iterating over all meta-devices corresponding to the raid1? Having mixed technology devices (TRIM and nonTRIM) I assume discard should be switched off? So using lcm_with_zero would be more suitable?

BR Stephan


On 24.09.20 18:56, John Dorminy wrote:

> On Thu, Sep 24, 2020 at 1:24 PM John Dorminy <jdorminy at redhat.com> wrote:
>>
>> I am impressed at how much I read wrong...
>>
>> On Thu, Sep 24, 2020 at 1:00 PM Mike Snitzer <snitzer at redhat.com> wrote:
>>>
>>> On Thu, Sep 24 2020 at 12:45pm -0400,
>>> John Dorminy <jdorminy at redhat.com> wrote:
>>>
>>>> I don't understand how this works...
>>>>
>>>> Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
>>>
>>> Yeah, I had same question.. see the reply I just sent in this thread:
>>> https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
>>>
>>>> I think also limits is local to the ti in question here, initialized
>>>> by blk_set_stacking_limits() via dm-table.c, and therefore has only
>>>> default values and not anything to do with the underlying queue. So
>>>> setting discard_granularity=max(discard_granularity, chunk_size_bytes)
>>>> doesn't seem like it should be working, unless I'm not understanding
>>>> what it's there for...
>>>
>>> You're reading the dm-table.c limits stacking wrong. Of course DM stack
>>> up the underlying device(s) limits ;)
>>
>> Yep, I failed to read iterate_devices...
>>
>>>
>>>>
>>>> And shouldn't melding in the target's desired io_hints into the
>>>> existing queue limits be happening in blk_stack_limits() instead?
>>>> (Also, it does lcm_not_zero() for stacking granularity, instead of
>>>> max()...)
>>>>
>>>
>>> DM core does do that, the .io_hints hook in the DM target is reserved
>>> for when the target has additional constraints that blk_stack_limits()
>>> didn't/couldn't factor in.
>> Yes, I had erroneously thought the limit-stacking was after getting
>> the targets' individual limits, not before.
>>
>>>
>>> And blk_stack_limts() does use max() for discard_granularity.
>> ... I'm just terrible at reading this morning.
>>
>> Thanks for pointing out all the things I misread!
>
> Actually, though, I don't understand why it should be max instead of
> lcm_not_zero(). If the raid's chunk size is 1024 sectors, say, and
> you're constructing it on something that has discard_granularity 812
> sectors, say, blkdev_issue_discard will be generating 1024 sector IOs
> which will work poorly when passed down to the 812-sector-granularity
> underlying device. While, if lcm(812,1024) were used, lcm(812,1024)
> sector IOs would be compatible with both the chunk size and underlying
> device's granularity, perhaps? Maybe I'm missing something, but I read
> the doc and code an extra time around this time ;)
>
>>
>>>
>>> Mike
>>>
>>>>
>>>> On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka at redhat.com> wrote:
>>>>>
>>>>> This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
>>>>> The reason is that the function raid_io_hints overwrote
>>>>> limits->discard_granularity with zero. We need to properly stack the
>>>>> limits instead of overwriting them.
>>>>>
>>>>> Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>
>>>>> Cc: stable at vger.kernel.org
>>>>>
>>>>> ---
>>>>> drivers/md/dm-raid.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> Index: linux-2.6/drivers/md/dm-raid.c
>>>>> ===================================================================
>>>>> --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
>>>>> +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
>>>>> @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
>>>>> * RAID0/4/5/6 don't and process large discard bios properly.
>>>>> */
>>>>> if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
>>>>> - limits->discard_granularity = chunk_size_bytes;
>>>>> - limits->max_discard_sectors = rs->md.chunk_sectors;
>>>>> + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
>>>>> + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> --
>>>>> dm-devel mailing list
>>>>> dm-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>>>
>>>>
>>>
>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dm-raid-set-discard_granularity-non-zero-if-possible.patch
Type: text/x-patch
Size: 1694 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20201218/23c741a6/attachment.bin>