Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <03a87103-0721-412c-92f5-9fd605dc0c74@oracle.com>
Date:   Mon, 4 Dec 2023 09:27:00 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/21] block: Add fops atomic write support
Content-Language: en-US
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,
        chandan.babu@oracle.com, dchinner@redhat.com,
        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-api@vger.kernel.org
References: <20230929102726.2985188-1-john.g.garry@oracle.com>
 <20230929102726.2985188-11-john.g.garry@oracle.com> <ZW05th/c0sNbM2Zf@fedora>
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZW05th/c0sNbM2Zf@fedora>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bVpIbVkrY1pQSmxhZGlRRUszbWdUbkMwRFc2U0dPRU5BOVdSY3huVWlkcW1n?=
 =?utf-8?B?akZxVk1FZGtFUWhjckJnSHU2cE1rZVByMFF4V1gyVnBxRFdsdHJBS09SQXNJ?=
 =?utf-8?B?bXhBOHRQWGNtYXA3UHZZb281c3pUYkV1OXl2WXNuKzBwVWFXN1pmMUZxeXRK?=
 =?utf-8?B?R2VyeXJibmZHNDBrbkZHVzBhazBNTTVERVJRWUkxVmxlNTY4U2EwZTBiNmFw?=
 =?utf-8?B?RXZhVENoOTdTZDVLUnRXcndvS254RktXRkQ1cUREL2NKd2FPcGtyc3BKVG44?=
 =?utf-8?B?cCtUVWt6S3JWcW1zU1NONkVhMi9McFZiRTRTeHFSd285RUpiQmwwN3pXTHRM?=
 =?utf-8?B?cHhxVW43d1M3d29aa0NLTWNNV0QxdFpLd1h2R29wSG1RZ3Jseng2UURsTU0x?=
 =?utf-8?B?bVh0K0VpbUUyRFJ5UmxET3hvRjdtSlVNQXM5TGlGcUg2YnVOTVVOV25JRnU4?=
 =?utf-8?B?L2xBeVNabUR6NzVmeE0xREI4cy9CS3hzZ1ZwSWNvZ0pjUGtsTXhXOVFORm9E?=
 =?utf-8?B?SXhnSTUvQVhRbTdhWnAwczkwWlN4N1ROelAzWXIyZDhhb0NobnpaaGl0R3JE?=
 =?utf-8?B?cFdrOEx3RHlnUExkalg2Q2lmcDRCeWJjcm1NakNmU2IvOWhrNGIwNEtxYlRh?=
 =?utf-8?B?SDhMMGl2c1p3c25Wa1FiUys5MmtxbG0vZnhhVm9RODNZbGdCUDJDQXljVHAz?=
 =?utf-8?B?ekE5aHZJMVo0emUzSEpEK2JPWU5CZDREMGkwbGpxNDhlRnpvRmRmSlR0YUJD?=
 =?utf-8?B?TSt5MVVPY2JFbnMzTUNFcnYyNmpYRkJPcXQwMG1Yb3BxR3FEK3ZmT0dDOEpM?=
 =?utf-8?B?Sm5URjZySjNMVlpCZjE5cGhrRWh4d3JXOWRJUmE2RVVSenp4c2hOeXRsZ3Vn?=
 =?utf-8?B?V2dpUlRPUkF1c0xTenY5NzF6MVp1SkNhdkFqZmpvSzdjUUxqVW5TUHljTDl6?=
 =?utf-8?B?dmtSa05DZDlUOTZpVm9SempnVG5XQW1XZlBaeHo3aGRUVzVpTDVhVHJ4QlNT?=
 =?utf-8?B?aDNEeTVVYWE5RktXLzc1OTM1ZlRwRlF3NnRXRUJsYmZNNi9FNkFKS3B1VWNF?=
 =?utf-8?B?eEFYbFgzZmttTGFaY1dJUVJFYk8zdGRYa0R3YXBzZWpIWG0vYnlub25xQ1Rr?=
 =?utf-8?B?NW5BY0dFOEIzcEcrK2tKaXFndVEwYWlhaXBMZVdkWElNZWlnSGVFZzV3K2xa?=
 =?utf-8?B?SUxheXNrRVNIMkhOWGp4NWdqOFFJWEsrK3pmZjllb0RxL3NNMjZyVHdVanMy?=
 =?utf-8?B?azJVT2NTbUNkVUxncWF6MURrc09LYzNjV0I5NlJIaUNUV3hRang3QXRBOFdZ?=
 =?utf-8?B?NTZVbDZCcmtSLzBOTjNLK2EwZ2pBaHZJZE01TWJjSUVLdm42VlBQblhLK2NC?=
 =?utf-8?B?Y0N6a0h5MStlY3ZZM01sRzNnKy92MkwwT0tSckFaMUxKT29kOFhuN08zTmhr?=
 =?utf-8?B?QzVZTkYyRWNJSmVxdTVWTlVXRmhiTFRad3dWYXVQTnl6OFRMTEcyS1lFcWhH?=
 =?utf-8?B?U1hwR2lKTWJXRmlUM2NyQkNUMUdnY2xsWGI1N2JsZjRhdEhKYUo3NXI0MW11?=
 =?utf-8?B?NUlIc2JuUExRYkpyTmh5TURZR280cytSWmpxa0p3SlpyRjhmcGhNNlhvU1NG?=
 =?utf-8?B?NE43R3RUNVpicUwwUzhrTG91dUhaQWJCVFZvanFZSFFYZU9Xc1pjeW5mQkxz?=
 =?utf-8?B?bUhpVys2UXZGV3B1d05RK3Z5c3AxQnorTS9qSEQyUjRLU2Y2ZkovTy8rU2Mr?=
 =?utf-8?B?NTJncXRZVkVFQ1FYUlR3MHJSQkp4bnN4b3RHUXNrcDFkTTc5RVRTakhiSFJH?=
 =?utf-8?B?STJQNVNOd3RDcE1qTUMwVXU5Z2RKVVJpNmtYNVpIUUNubXFQL1pSSkV5dFds?=
 =?utf-8?B?SHVzSFJySzZ3TUZBeTNqMWUrOU1PRy9lTmVwZkhZU1NzWkk1WWk2ZGlwMFdQ?=
 =?utf-8?B?YWhlNlB3QWdUMHBRUkxjS3pXWStoenorUDRMVS9aeUwybXFuYmprSWtTWm1s?=
 =?utf-8?B?ZE5VUDNWQzI0d3NwbGovTFZkTnlndTIyUi80NlBSUkVIZ25kWGk3cjc5YUVU?=
 =?utf-8?B?aHp4bDZ3NkxGa3dzREprdWRVUGNjUDZ3UkJGeXNYKzdPWFhXMzNZUjZpSno2?=
 =?utf-8?Q?NMgPCqurv6VrnSn9+r3FtXq99?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?YStqVk03c1RQUStESHJVbDBzb2hmTVRydHFaYlMzbSt2Zm9McTkzUkJ3cmtQ?=
 =?utf-8?B?RzdiT1NpVzI0Vk5Id0hYVjV6S0MvQlFyNUZIZmFTZU1BaXErU0luNjRqR1BY?=
 =?utf-8?B?VnV3RmFtTzRQd2xBWEw0VHJVemlFTm5DSHVUcHN0bGo5WTVTUVhUUnI5OFhF?=
 =?utf-8?B?eVpmdm5UVThCK0MwSHRKMzZsem1EZVZqZ1pMNlZSYlBhVmhzbXBkZ3lUK3kx?=
 =?utf-8?B?YkZIU1lab042dUZpNWtlSVNnUFdZUUpLSUtpZ0drZ092U21qRkFUaEpkYUI3?=
 =?utf-8?B?SXV3WWVGd3daSytwTDhEY2Y4ejJmNzBoMk8rMWYrN3BaWHBIbEJjU2paWHBy?=
 =?utf-8?B?K25jdEpvOW15bTA3N2ZOSnJodms1ZWkxOS9iOTkwSTNSYXdjYlNva09EZXRx?=
 =?utf-8?B?cUpqUHdyTEZndEt6L1o3bW5oVThPZ05PYlhjQk9HK3FIcm1hMVJRQkRGcHl2?=
 =?utf-8?B?cnBmSzArT29hek9YNzBmaUh5QkJyRlBkcHZCT0VudVl2MDNFY3dLRmd6VG9o?=
 =?utf-8?B?N1E5ZzhyeVIvYjZOUXlOTXB6WDhmYUFqNTVjb1BlRnYwVU1wL0V4c3R0WnRz?=
 =?utf-8?B?OXBqdGQyRUFtbTJBUUhqK1RVQUxqUjlwM1gwLzdxV1p4MHhuWDB0dkY4VlFx?=
 =?utf-8?B?azFhTHZHVTJyck1zWnVSZ2VLVDhpL2lFNDJSVDJjUHNBVWQ2S3FlMy93N1lH?=
 =?utf-8?B?b0tkcytCcG5GanRUOFRHcGVpNkw5ZEcvNE9NOHZYUUd6TnlnT2QrSWF0REox?=
 =?utf-8?B?bWZTa1g0cXJHa0x5bjM3aVFIUGhLMFlaQnZxKzlSNUl2bTAyMFlBTzROQzll?=
 =?utf-8?B?eG8zYlVWVUM3SUdZLzFrWDAxZDJqanZ1Q2x0UExKUVZZRHRWcTVYRnlUeUll?=
 =?utf-8?B?TE1QdktvSzI0V0RPWTQzamVkRmc4YkxVbk0wTE9tdnBIQnpFOWx6ZjBpOEZM?=
 =?utf-8?B?MzQrcTVocitLMmJ6bEJQZmxSeUFpeUcwR0tILzcyVmE0Q3Erd3M1NW00YUsv?=
 =?utf-8?B?RGNCYXE2bHB1MFVseHozL1FYcUdrR2p6SHpDSFFpK3FRTllWZjFzbUxzN3N6?=
 =?utf-8?B?VE54ZVR1di92WUYrQXJwRjlDYitiSkFEVy9yallQNkdyU0hYRFU1ZS8yV0tB?=
 =?utf-8?B?SVI4ZXRralRTeXQ4QyttWWR4N1dsbnFvUlo3VlhKVWF5ZGpCWVR6T1gwUlor?=
 =?utf-8?B?aUVMbkQxWDZ2NkJOd1BsOFpyNHJGQVBBeFN4ODhHeDU0dkc2S0dwa3FMYnhz?=
 =?utf-8?B?N0M4MmppTWw4eUZCWmJOb0RQWldNSENqWTJZNXBGZzNrSEU1UmFZU0ErdUZy?=
 =?utf-8?B?ZFB2clVNUDkxcVF1QnlyWmNUbmJiTUxqaHBsZ0Q4dUF2c0lzMDVicm5LQ0t6?=
 =?utf-8?B?b2FSL3g3VEhlQTJzdWpCaWdUVE9sc0lCZXgzNlM5RElBa2trMEsxQ05ZaGFs?=
 =?utf-8?B?NFV3N1BtemxpRnBra244aXRWMkhWdGpSOCtIQlBRPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e17f6d68-81aa-411d-0969-08dbf4ab2e3a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Dec 2023 09:27:05.7153
 (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: OQGsHtfjfmijZRYE5SSNDzqBb1Wu+X5PsetSp78c6FkGeQY7wy9j29t9JDRCUe1DfHM/5Yko1WdtBb6UrmhD4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4521
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-04_06,2023-11-30_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 adultscore=0 phishscore=0 malwarescore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2312040071
X-Proofpoint-GUID: lq9WZejnx554_9JE7ZNA1YXnizVtAzIZ
X-Proofpoint-ORIG-GUID: lq9WZejnx554_9JE7ZNA1YXnizVtAzIZ
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 04 Dec 2023 01:27:53 -0800 (PST)

On 04/12/2023 02:30, Ming Lei wrote:

Hi Ming,

>> +static bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos,
>> +			      struct iov_iter *iter)
>> +{
>> +	unsigned int atomic_write_unit_min_bytes =
>> +			queue_atomic_write_unit_min_bytes(bdev_get_queue(bdev));
>> +	unsigned int atomic_write_unit_max_bytes =
>> +			queue_atomic_write_unit_max_bytes(bdev_get_queue(bdev));
>> +
>> +	if (!atomic_write_unit_min_bytes)
>> +		return false;
> The above check should have be moved to limit setting code path.

Sorry, I didn't fully understand your point.

I added this here (as opposed to the caller), as I was not really 
worried about speeding up the failure path. Are you saying to call even 
earlier in submission path?

> 
>> +	if (pos % atomic_write_unit_min_bytes)
>> +		return false;
>> +	if (iov_iter_count(iter) % atomic_write_unit_min_bytes)
>> +		return false;
>> +	if (!is_power_of_2(iov_iter_count(iter)))
>> +		return false;
>> +	if (iov_iter_count(iter) > atomic_write_unit_max_bytes)
>> +		return false;
>> +	if (pos % iov_iter_count(iter))
>> +		return false;
> I am a bit confused about relation between atomic_write_unit_max_bytes and
> atomic_write_max_bytes.

I think that naming could be improved. Or even just drop merging (and 
atomic_write_max_bytes concept) until we show it to improve performance.

So generally atomic_write_unit_max_bytes will be same as 
atomic_write_max_bytes, however it could be different if:
a. request queue nr hw segments or other request queue limits needs to 
restrict atomic_write_unit_max_bytes
b. atomic_write_unit_max_bytes does not need to be a power-of-2 and 
atomic_write_max_bytes does. So essentially:
atomic_write_unit_max_bytes = rounddown_pow_of_2(atomic_write_max_bytes)

> 
> Here the max IO length is limited to be <= atomic_write_unit_max_bytes,
> so looks userspace can only submit IO with write-atomic-unit naturally
> aligned IO(such as, 4k, 8k, 16k, 32k, ...), 

correct

> but these user IOs are
> allowed to be merged to big one if naturally alignment is respected and
> the merged IO size is <= atomic_write_max_bytes.

correct, but the resultant merged IO does not have have to be naturally 
aligned.

> 
> Is my understanding right? 

Yes, but...

> If yes, I'd suggest to document the point,
> and the last two checks could be change to:
> 
> 	/* naturally aligned */
> 	if (pos % iov_iter_count(iter))
> 		return false;
> 
> 	if (iov_iter_count(iter) > atomic_write_max_bytes)
> 		return false;

.. we would not be merging at this point as this is just IO submission 
to the block layer, so atomic_write_max_bytes does not come into play 
yet. If you check patch 7/21, you will see that we limit IO size to 
atomic_write_max_bytes, which is relevant merging.

Thanks,
John