Received-SPF: pass (google.com: domain of linux-kernel+bounces-1049-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <811039c5-8b46-4b99-9930-8494f5eaa2c4@oracle.com>
Date: Fri, 15 Dec 2023 13:55:22 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 08/16] block: Limit atomic write IO size according to
 atomic_write_max_sectors
To: Ming Lei <ming.lei@redhat.com>
Cc: axboe@kernel.dk, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me,
        jejb@linux.ibm.com, martin.petersen@oracle.com, djwong@kernel.org,
        viro@zeniv.linux.org.uk, brauner@kernel.org, dchinner@redhat.com,
        jack@suse.cz, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
        linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        tytso@mit.edu, jbongio@google.com, linux-scsi@vger.kernel.org,
        bvanassche@acm.org
References: <20231212110844.19698-1-john.g.garry@oracle.com>
 <20231212110844.19698-9-john.g.garry@oracle.com> <ZXu5rykouOcNOSa1@fedora>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZXu5rykouOcNOSa1@fedora>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?Yi8zQnpOQ0N5aU1vdTV4KzhSeVBsUjlCdEZqaXNxVm8yM0hyRzEzbWxpTndu?=
 =?utf-8?B?SnlqR2ROclgvaU44WmQ5S0xtQ0hoUXhPT1ZLenJzNTdIUnVWWGxUUkVJcFZu?=
 =?utf-8?B?UkgvT1lGWmpqVENxZUlyWVdmTmVXbElOSDIvb1EzVXRucWMySVN4OWpRdmV2?=
 =?utf-8?B?bWNTajgwL2FzZkFiZGJzT3J6TFNnbDNGbVZvNnEvSWxBa2JEakk0N1JNQ1Zi?=
 =?utf-8?B?b2d5WFIrNTM1U2p4b1UwQ2ZMWkh2WG9MaE9vK3pGcEJJQlRYQSthNWRCejAw?=
 =?utf-8?B?U2g2V3NYYjBWNE5QTHU5SklBNEEzcDBOM2Vha25wMGRYZmNkc2xKNXZydlBu?=
 =?utf-8?B?NHErTVh0WlVCck5FL2xOeHoydDJ4b3MyeFZqSjJ0eVBYNlBCbTlBY0pBRi94?=
 =?utf-8?B?M1BnVlJlVndWY1NvVG1Eb0hFa1ptcCtFeU1VWk9hazZadWhIMmFPY0lYdTky?=
 =?utf-8?B?TlY4L0FJYkhWZlNPaWU0RDRDdHBmcUpTeVdzZGZYdk5odXhCVUFZUUVNUExk?=
 =?utf-8?B?enQ0VkVBNVIxaXZvRnlZQVBhdlQ3eTI4L2pWZUwyUWtodCs1cGFuQmtGR2Fx?=
 =?utf-8?B?bzZBcHNXR2tNQlNmUjZETCtIUjJPR1ZEK1VDM0loamRzTEg1Wk9uSmh3NGFy?=
 =?utf-8?B?QVd0N1RqSjZlWm5DbmVldUpPVVRvZjVMMmk4Q0kzYVJ4RVJJU1c5TDNEeElW?=
 =?utf-8?B?ekZkZEJDcXF1b0RNK2l3QkFqU0tqTk5TME4zdUJQV1VRcHFTYkFMcXdoOEd0?=
 =?utf-8?B?Wmpsbll4MDFHbFUxdGc4OHYyekJMY2FQcG1yZ0VVN0xzY0MrVHg2NVhXWmV5?=
 =?utf-8?B?TEZLbnZyRVV4a01JSDJ3M24yT0JjU3lxNk1qd08xdS9TMC9KTDg4NTVkWFBP?=
 =?utf-8?B?NTBoNVBkSTYvRnFkNnRWWUZLVm1BUFpyaVRHWWZhdzlROHdJL1g3Q0hTRGcz?=
 =?utf-8?B?Uk8zZnhLSnVRRGdYdU1VeUw1UzZldjlnczJtUTdkTlBkK3J3OGtUSVhyU1lC?=
 =?utf-8?B?VXRrRnVuRktHcGFENFdNdGthaXFHejhlMFdKNTNXbkxmYXE5Z2lzNE1rb1Rw?=
 =?utf-8?B?UTU2Z1FaTm1UWVVpV3o4WHZSZXo3MVdSdVppcDVHTlJ1WHRCSXkybEd4Tm1q?=
 =?utf-8?B?dWQwNVp3Y0JLVFplMkw1d2V2ZDNMV0NiaTdLR0tLbU9RMFNrd1dsYjRoVkJo?=
 =?utf-8?B?ZW82SlpnV3MxeVN3Vm5aVVdYZXdmTWVPanRuWjVIRWRKdkxjWUdERk9xWFdG?=
 =?utf-8?B?b1ExdVEvTEZCRjBTcTY4SGtlbnBaaVA0am5qcU5VN0srZXNzQjNkbEcwOVVy?=
 =?utf-8?B?ek1LL1ZYTTNhVlp0b0gzMXdGblpYZ3FSdHd6eWdRT1Y5dDgrblVnN21DejFL?=
 =?utf-8?B?eFlSbE5kQTlwbUdGTGhHclhCSlFRbEN1dHpNSWdIT1ZDWThJZGtHVzNzMWRM?=
 =?utf-8?B?RnM4VnIwbHpSTVkyMGtFTmVrWUpycW5Td3RDVUpxbmJIUWlWMGRlNWZ1OWhh?=
 =?utf-8?B?Y0xrWkRoZEJzUDRQaFpYZTM1M3cxN0dJZm84VTJzTUM2c0RDa0RnNWFzSnRl?=
 =?utf-8?B?KzFSNGFFYWJzWWhINHhqTGRBK0ZUamJIampLUHpkL3hGRC9MZkpxUTNwcmpJ?=
 =?utf-8?B?TlQxN2JLMzJIUldROXNjOGhqUmtHMmxydnFuVUJSSFg3WHlmd25rUWM4a1p4?=
 =?utf-8?B?QllJSlM4aXZmd0JxU1FvNTNxb05YUnRBZDZIOFJ2bkp5TkF4NGtzRzc4bWZW?=
 =?utf-8?B?WXN0YWl0QXhETENpb0hwRnNYYjNabGZ3WG13bHo5Nk5uZjdIL0dmN0s4VDFG?=
 =?utf-8?B?YWdXTSsvNTBrc3BCMzcvTHZwM0ZFOGZzeDQyOWZQUzdtOTZaakdOQWx6RTQ1?=
 =?utf-8?B?aHdlaTk1Q1A1NlE1OURaRTRKQU9iL3ZDYStxc05waXlxNGxDZ3MzdnR0TjBP?=
 =?utf-8?B?QjlrMG1ya3hBNzh6cUROcFZMM0dneVZGZCtvRnFNdEF6ZXhSWC9BMlhndkZz?=
 =?utf-8?B?WEVOT2s5YkhHOXZRcm5IYjljUmoza1hFZ0lKa3NEOWNKdlRueHc4bURvQmUx?=
 =?utf-8?B?Rjh4SzFHYmhjVS9sUEhVMVlsUW5FNFhqWjBDeThVbVVtTTBzSFFxcHNXSnBk?=
 =?utf-8?Q?HHlH5mkbBVqD+6n28MLqGA/sz?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	rV0NBfs1rpwOywVpCf5Wcggejbo3H7HvYQ7RyYdB+HxaRK9HtG2qTfGPqTrM818bvNpliaIAMF+lV5SFi+9GkKCtwWxouUrvhOW6vOUAd3TAI8ajDDomM2pWdwqvuP3XQFVkREwjQ6w7CbvUTJi1RTH8OQdk7qiiIRZWdd17Ylg3znBfDelspq3OTny+CkkLTFpPwcF3zfwuRT9fJJblpjqK5DUEaSnlqdKjv0MKJHSE0aUu+VY6IMFyYdJ7BYs5p4pkQG4X5NNGmRA3fmNXrPcVVEn/lmKxtwRj0QuAbY7/+WXTeb6EDVW6WvxV8ji/4tVdS5j3ym2KjHb4bHsPZABC0oqKnHCjOpiBxphfq983qSiOAzauKAohEAD2Hm0GSjRC1nMxhPxAmktCf5UepZcyRfGu/e9EyfexCqfzu/9Ibp7v7QEGoG5T6+yFCUkObnSC0IoLhP54bNvVRXkuEaW6ifk/1JPyWsU2eLDTZxLdsLt963aZ3/WyvbHU8Hdm00sGqwM4uxqUo/g2yNiII358Jm2r1eh4n0sGBgBKe937t0PyDFcOdWiL1Zpx6UIcsRda6LrrggpuVeQwtkx4+Q62kR8PcVy1lJWawHrcjjo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 31c94e64-9b33-4da6-6493-08dbfd757eb6
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2023 13:55:28.4774
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RvWlcCNgzyYXVDq6QkqeTQ0VhfQ3w+MGYgJB3rz8+PU/KeDO00+0IVE3tPd4/uEzgXUdaa+PbsZQiMXH6U4bLw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4455
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-15_08,2023-12-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 suspectscore=0
 phishscore=0 adultscore=0 mlxlogscore=999 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312150094
X-Proofpoint-ORIG-GUID: JvGzuvL4ibmtkbZAh2DlHpPb2Y_XfM0V
X-Proofpoint-GUID: JvGzuvL4ibmtkbZAh2DlHpPb2Y_XfM0V

On 15/12/2023 02:27, Ming Lei wrote:
> On Tue, Dec 12, 2023 at 11:08:36AM +0000, John Garry wrote:
>> Currently an IO size is limited to the request_queue limits max_sectors.
>> Limit the size for an atomic write to queue limit atomic_write_max_sectors
>> value.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   block/blk-merge.c | 12 +++++++++++-
>>   block/blk.h       |  3 +++
>>   2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 0ccc251e22ff..8d4de9253fe9 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -171,7 +171,17 @@ static inline unsigned get_max_io_size(struct bio *bio,
>>   {
>>   	unsigned pbs = lim->physical_block_size >> SECTOR_SHIFT;
>>   	unsigned lbs = lim->logical_block_size >> SECTOR_SHIFT;
>> -	unsigned max_sectors = lim->max_sectors, start, end;
>> +	unsigned max_sectors, start, end;
>> +
>> +	/*
>> +	 * We ignore lim->max_sectors for atomic writes simply because
>> +	 * it may less than bio->write_atomic_unit, which we cannot
>> +	 * tolerate.
>> +	 */
>> +	if (bio->bi_opf & REQ_ATOMIC)
>> +		max_sectors = lim->atomic_write_max_sectors;
>> +	else
>> +		max_sectors = lim->max_sectors;

Please note that I mentioned this issue in the cover letter, so you can 
see some discussion there (if missed).

> 
> I can understand the trouble for write atomic from bio split, which
> may simply split in the max_sectors boundary, however this change is
> still too fragile:
> 
> 1) ->max_sectors may be set from userspace
> - so this change simply override userspace setting

yes

> 
> 2) otherwise ->max_sectors is same with ->max_hw_sectors:
> 
> - then something must be wrong in device side or driver side because
> ->write_atomic_unit conflicts with ->max_hw_sectors, which is supposed
> to be figured out before device is setup
> 

Right, so I think that it is proper to limit atomic_write_unit_max et al 
to max_hw_sectors or whatever DMA engine device limits. I can make that 
change.

> 3) too big max_sectors may break driver or device, such as nvme-pci
> aligns max_hw_sectors with DMA optimized mapping size
> 
> And there might be more(better) choices:
> 
> 1) make sure atomic write limit is respected when userspace updates
> ->max_sectors

My mind has been changed and I would say no and we can treat 
atomic_write_unit_max as special. Indeed, max_sectors and 
atomic_write_unit_max are complementary. If someone sets max_sectors to 
4KB and then tries an atomic write of 16KB then they don't know what 
they are doing.

My original idea was to dynamically limit atomic_unit_unit_max et al to 
max_sectors (so that max_sectors is always respected for atomic writes).

As an alternative, how about we keep the value of atomic_unit_unit_max 
static, but reject an atomic write if it exceeds max_sectors? It's not 
too different than dynamically limiting atomic_unit_unit_max. But as 
mentioned, it may be asking for trouble....

> 
> 2) when driver finds that atomic write limits conflict with other
> existed hardware limits, fail or solve(such as reduce write atomic unit) the
> conflict before queue is started; With single write atomic limits update API,
> the conflict can be figured out earlier by block layer too.

I think that we can do this, but I am not sure what other queue limits 
may conflict (apart from max_sectors / max_sectors_hw). There is max 
segment size, but we just rely on a single PAGE per iovec to evaluate 
atomic_unit_unit_max, so should not be an issue.

Thanks,
John