linux-kernel - Re: [PATCH V4] block: fix the DISCARD request merge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 27 Oct 2018 19:46:10 +0800
From:   "jianchao.wang" <jianchao.w.wang@...cle.com>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     martin.petersen@...cle.com, tom.leiming@...il.com, hch@....de,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] block: fix the DISCARD request merge

Hi Jens

On 10/26/18 10:25 PM, Jens Axboe wrote:
> On 10/26/18 3:28 AM, Jianchao Wang wrote:
>> There are two cases when handle DISCARD merge.
>> If max_discard_segments == 1, the bios/requests need to be contiguous
>> to merge. If max_discard_segments > 1, it takes every bio as a range
>> and different range needn't to be contiguous.
>>
>> But now, attempt_merge screws this up. It always consider contiguity
>> for DISCARD for the case max_discard_segments > 1 and cannot merge
>> contiguous DISCARD for the case max_discard_segments == 1, because
>> rq_attempt_discard_merge always returns false in this case.
>> This patch fixes both of the two cases above.
> 
> Looks good to me, but:
> 
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 42a4674..cf817c7a 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -714,6 +714,32 @@ static void blk_account_io_merge(struct request *req)
>>  		part_stat_unlock();
>>  	}
>>  }
>> +/*
>> + * Two cases of handling DISCARD merge:
>> + * If max_discard_segments > 1, the driver takes every bio
>> + * as a range and send them to controller together. The ranges
>> + * needn't to be contiguous.
>> + * Otherwise, the bios/requests will be handled as same as
>> + * others which should be contiguous.
>> + */
>> +static inline bool blk_discard_mergable(struct request *req)
>> +{
>> +	if (req_op(req) == REQ_OP_DISCARD &&
>> +	    queue_max_discard_segments(req->q) > 1)
>> +		return true;
>> +	else
>> +		return false;
>> +}
> 
> Please get rid of the redundant else.
> 

OK, I will change it and post next version.

Thanks
Jianchao