Received-SPF: pass (google.com: domain of linux-kernel+bounces-80978-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <416e3bea-9a1f-46ac-8bd9-4455358e1d9e@oracle.com>
Date: Mon, 26 Feb 2024 08:51:18 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 03/11] fs: Initial atomic write support
To: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>, 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
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-nvme@lists.infradead.org, linux-fsdevel@vger.kernel.org,
        tytso@mit.edu, jbongio@google.com, linux-scsi@vger.kernel.org,
        ojaswin@linux.ibm.com, linux-aio@kvack.org,
        linux-btrfs@vger.kernel.org, io-uring@vger.kernel.org,
        nilay@linux.ibm.com,
        Prasad Singamsetty <prasad.singamsetty@oracle.com>
References: <87v86d20ek.fsf@doe.com>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <87v86d20ek.fsf@doe.com>
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?aUFxdXRGVmpncW1zeTNUOS9FVVFPa3YvVE9PM1VoR3hVcjZpS2QyNnREYVBI?=
 =?utf-8?B?WEtUQlZnUzh1blZQYnhlUjliOE8xQkE4cUJ3MWFNQlpISWlnbStrNlQ3cXlS?=
 =?utf-8?B?OFVSYkNCTVRIcG02UlkybFpQOVNwWkxBSFhvR1pSdmtpdVAvdHFuT2FzVGF2?=
 =?utf-8?B?U0F0WUUwdDNpMlJwMnRKT0N0cHNwUnYrNUJrK2Q3MU1hUjQ3eHhKWmhHTEhq?=
 =?utf-8?B?Y0x0am1JSjZwMTZ5R1hFTjZKWm5VbjlwWndnK2xYUTlLQnRTaFM5djNDYjNH?=
 =?utf-8?B?SFRmRE9NMi9EdldIOGNQYkdwbEFzYjR3WG5Ibm5SdGkwUWxySmxtaEVNTUsy?=
 =?utf-8?B?M2dEWUJBNzBnOUpqVklOTTMzc2szempGeVFmUk04VzZhZExVVHUybm1HR3FH?=
 =?utf-8?B?RFJqYkIzdjlPUEZ0QkdmeFJrN3ZoUWVPWGJkRFFiR0tSVlUyZVpTTGloYjR3?=
 =?utf-8?B?bDhvcUt2VGx3S2NQNGZQY21oc01JbHozYmltQVh3VDFJYVpRblh3ZGZ6enBH?=
 =?utf-8?B?TVoxbmpHcWtWZHppVWdXQjZMdFJ4T3RTREFZcmFwS0ViZzc2YnZFN0dkU2NN?=
 =?utf-8?B?cjg2SFgwbTRyLy9aaDlzQTB0UWoxeGhuck9MQzdneTdrMm5vSUhsMG5uN2JD?=
 =?utf-8?B?TGFoTTk3ZjNoWUhOUm9jd3JMRzc1RWtsMmdQaUZlcnFubXNnWERsQzJkT3ZG?=
 =?utf-8?B?ZEdZUWkwMDZSL2JSK0gvUXdyN1QvWkdXZEVaVm5sU21YTUl4WHBpcEhsNnZT?=
 =?utf-8?B?OG1oZ2tDK1hvcWFYV2JEbnZnWUJyekg5elphYW5qNVYxVG5OaDVYejBCaUM4?=
 =?utf-8?B?Nm5POTlkVzErZXcyRWRxUHVGVXJkRnh0bXkwQXJhY20yNlg2elowNHFydEdF?=
 =?utf-8?B?NnAxR3ZmeHZrL25xLzBOZEFVMFFsU1hQWFhua0V0czB3VUZaL29Fam00VktL?=
 =?utf-8?B?Zm9OMjNTeW1JUHBzV3p5ZjhOQ2E3Ym9wQTlHQ1FvTzR0M25OSmFaWUVOM3Vy?=
 =?utf-8?B?NXhxa0cvenVRZXc1aWF2a3hnWmlTeUJORkV2blhJMXpmUXJma2d6bmVQaXZs?=
 =?utf-8?B?SzZUVGZscjFYMWpMZjdQdStGZG5LdTRnQ2orb0IvZDVBNmMwZHJHaWRWakNl?=
 =?utf-8?B?L2FPdlFMem1oZDAxbGMrWitxV2FlUFhuLzRTL1psZVpFR0JEc3owR25sZjd0?=
 =?utf-8?B?UVNzQ3UwcXpVYWNqVXl3djQ1Sk5nY2NGU0prbzZ0QVh6Q0Rab2hiSjQxNk5w?=
 =?utf-8?B?UDZ2aSt0UllZVC9wTEsvTi9zYkdKSVNOTDBxOUFDN2VKQkx5QVBQczkxR0Vo?=
 =?utf-8?B?ajcwQzU0WjBrNklCU3ZydHQ5NklaMnZqSnJ0S0JCZ2hlU3VWZjY4Q0M0Vmxm?=
 =?utf-8?B?WmpoRlJaMnk4TnY1OVhlL0lwa3g5TFkzYkFuVnFnVGtPNEkyaVFaODgrTktD?=
 =?utf-8?B?SndBQWdDbzBhM1RMdVcvbmVxYXRtbWtiLzE4WmltU0RPOTh5RnlwUkdpcXdW?=
 =?utf-8?B?aVNxYVpRUmFTNkdoZlpVM21uUFFta2lCZHRqcWFDZXhqMlE4OHhjNHEzcnVh?=
 =?utf-8?B?RlhSRWNBODF0R3JkVzBac2JCWGxWOW5BU1YxcUZxUUVDbEJ5aW9oeS8yNm9T?=
 =?utf-8?B?QWpUNFhENjlHZXhCKytrYU1RS0pNVGlKSDBadTdnaE92UzlUL0NOaGZTNU44?=
 =?utf-8?B?dUpxN0kxOWgyVnR1cEJIdGptL1drWEZJRWhHbkFuZGxSR0tSMHNONThZV0ZF?=
 =?utf-8?B?ZEs0dTE1elcxU1dIWlJHZllrK0xOUkRjR1RuczVScEZGSzRnckxNUkJUL0FR?=
 =?utf-8?B?RGtiLytlMjFWYTJ1cStMK0hsN2pTZ2xhRGwvMTJ2ckVNcFJ0RVpWaytBbTF0?=
 =?utf-8?B?TENUNFJJbzZVRjQ3MTVNTStCdXduOVdLeUhiZFBDajlZeXo4bXhFbEFSdCth?=
 =?utf-8?B?bldaOGFlR2ZoditGVTdjUk9NeGR3RTlwN2FydzBBNGRqQkpuM2NMZmc0S3E2?=
 =?utf-8?B?MENiUmtMcHI2emRyK1ZqUUJxZnZleUVybW9jbnB6QjJnOXd5VWl5VmhycEE3?=
 =?utf-8?B?K1lFR2J3RXBlTHZmNkJsUUVmbFpsYTRXd1E1cWZVbENFVGIvbUhWdDg0bUNK?=
 =?utf-8?B?VUR6dHNuY0FsRVlOaDR5WFFnTERQcExHYmxqVjhFenE5cFdVa1c5Z3kzdito?=
 =?utf-8?B?ZUE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	A+R0DzQVQS8LcUiaPS9TPPbGq3obgTNwpdVcpDxX/7HTBDUsA/aNYWKGQ5fNggWyEADElq+vSFuuhDgJbiKQcLmWsSOveMZniLUo8AvuCQtMqv5qLmFrXb0/if9gb5Fsp+uJQ6nBAcM2SFYklnyaQR9bDG6FQffzSQpBvrGvGB5Om/xt4XxiBLbAqCfI8XqnWc+SxrOoFOJA21vIAQyr33SVUIyrpMDwBGjBfpRO5wD2uNx4YBqQZsxlLxMCIwlf7FWjs2SLsfNB19KZ1LBpmG4q2sjxD1cQf44XGQWroJHa7oM104qGnQFOm5ju0e2mGJaxDYhN1//2l+NFp14SUGpTZrY7W1CaiZb7QLy8Jx0+hW2q0Ly7NbJdqWpwW7codvwqUqsjnyKx6vzpApPSTbSK/eYUEt8Y0Fxwe0fxUWAzgyMYq8u7R6J+/PWYB37HHV2VpYv6P4Fd69+qbUyu+CrBaulx07pKBXum7Y/S7p2Pm9Irjq7Ls1DehJXHoun1I/wR4UTi6jHFCZIknjCOwdNmoe1HIXXMVHwzPrc9i9RFOXwFSI88R98svsC/eke7EsF1FHowM7cQ/8QqjbZfEUSP/5ye1MIF5mbqJPyf0AM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 833418cc-ce51-4f09-e61c-08dc36a81c0e
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Feb 2024 08:51:23.5182
 (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: oRKbtgkDQ3HcwGybhAPD2GMdA+9rM99OqxBzOt7ulZRh7hGXIcSFof6kXSGghngKEX+fN4RM2HmcxqMPwdurhw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR10MB6428
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-26_05,2024-02-23_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 adultscore=0
 mlxscore=0 mlxlogscore=999 phishscore=0 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402260066
X-Proofpoint-ORIG-GUID: RdHCMKJv4UIQFbkWqKxZvFsprCBEQPZh
X-Proofpoint-GUID: RdHCMKJv4UIQFbkWqKxZvFsprCBEQPZh

..

>>
>> Helper function atomic_write_valid() can be used by FSes to verify
>> compliant writes.
>>
>> Signed-off-by: Prasad Singamsetty <prasad.singamsetty@oracle.com>
>> #jpg: merge into single patch and much rewrite
> 
> ^^^ this might be a miss I guess.

I'm not sure what you mean. Here I am just briefly commenting on much 
changes which I made.

> 
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/aio.c                |  8 ++++----
>>   fs/btrfs/ioctl.c        |  2 +-
>>   fs/read_write.c         |  2 +-
>>   include/linux/fs.h      | 36 +++++++++++++++++++++++++++++++++++-
>>   include/uapi/linux/fs.h |  5 ++++-
>>   io_uring/rw.c           |  4 ++--
>>   6 files changed, 47 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index bb2ff48991f3..21bcbc076fd0 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1502,7 +1502,7 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
>>   	iocb_put(iocb);
>>   }
>>   
>> -static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb)
>> +static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb, int type)
> 
> maybe rw_type?

ok

> 
>>   {
>>   	int ret;
>>   
>> @@ -1528,7 +1528,7 @@ static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb)
>>   	} else

..

>> +
>>   /* 32bit hashes as llseek() offset (for directories) */
>>   #define FMODE_32BITHASH         ((__force fmode_t)0x200)
>>   /* 64bit hashes as llseek() offset (for directories) */
>> @@ -328,6 +333,7 @@ enum rw_hint {
>>   #define IOCB_SYNC		(__force int) RWF_SYNC
>>   #define IOCB_NOWAIT		(__force int) RWF_NOWAIT
>>   #define IOCB_APPEND		(__force int) RWF_APPEND
>> +#define IOCB_ATOMIC		(__force int) RWF_ATOMIC
>>   
> 
> You might also want to add this definition in here too
> 
> #define TRACE_IOCB_STRINGS \
> <...>
> <...>
> { IOCB_ATOMIC, "ATOMIC" }

ok

I suppose that new flag RWF_NOAPPEND in linux-next also should have this

>>   
>> +static inline bool atomic_write_valid(loff_t pos, struct iov_iter *iter,
>> +			   unsigned int unit_min, unsigned int unit_max)
>> +{
>> +	size_t len = iov_iter_count(iter);
>> +
>> +	if (!iter_is_ubuf(iter))
>> +		return false;
> 
> There is no mention about this limitation in the commit message of this
> patch. Maybe it will be good to capture why this limitation to only
> support ubuf and/or any plans to lift this restriction in future
> in the commit message?

ok, I can mention this in the commit message.

> 
> 
>> +
>> +	if (len == unit_min || len == unit_max) {
>> +		/* ok if exactly min or max */
>> +	} else if (len < unit_min || len > unit_max) {
>> +		return false;
>> +	} else if (!is_power_of_2(len)) {
>> +		return false;
>> +	}
> 
> Checking for len == unit_min || len == unit_max is redundant when
> unit_min and unit_max are already power of 2.

Sure, but it was an optimization, considering that typically we will be 
issuing unit_max in anticipated FS scenario.

Anyway, I will be changing this according to an earlier comment.

Thanks,
John