Received-SPF: pass (google.com: domain of linux-kernel+bounces-59735-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <36437378-de35-48dc-8b1e-b5c1370e38b0@oracle.com>
Date: Fri, 9 Feb 2024 17:30:50 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/6] fs: xfs: Support atomic write for statx
To: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc: hch@lst.de, djwong@kernel.org, viro@zeniv.linux.org.uk, brauner@kernel.org,
        dchinner@redhat.com, jack@suse.cz, chandan.babu@oracle.com,
        martin.petersen@oracle.com, linux-kernel@vger.kernel.org,
        linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        tytso@mit.edu, jbongio@google.com
References: <20240124142645.9334-1-john.g.garry@oracle.com>
 <20240124142645.9334-5-john.g.garry@oracle.com>
 <ZcXNidyoaVJMFKYW@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZcXNidyoaVJMFKYW@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.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?c3c2NWt0ZmR5REJjU25MTlNmSGtDc2cxaWxWL3o5U2l4cFFaWVFSR0hEbFNI?=
 =?utf-8?B?dGs2NGVjVjJOcHpJaUNqUk56OERoT1NuaGoxSGFoMnA2K0U3ZmJQd2lCdW1M?=
 =?utf-8?B?NlBveGoyaWpadTZ5RW5IR08zckZIbFRCR0lTVlRBVlVQWUdxQ0pUQnBZS1pC?=
 =?utf-8?B?QVdTcXM0OW4yNFBDQ3dhME5sSDEzTklUNU9BQXJaT2l5dy9VSWw5a2ZOWHhE?=
 =?utf-8?B?VDVoblJYNFFJazFJQjRWWVJHMW00VjI5VmxUZDIrbDIvdEFqVW8wSEMwQk1h?=
 =?utf-8?B?NjJXcnRtUmNIQWJqWGpWdHdJeHUrSjRQeE90cjNHeVVnWVgrUFhCaFVCdUtC?=
 =?utf-8?B?enMzR0xVejQrRUhsQm43OCtzcGdZTTFyaU9XME9sak5naVJBK01LK1BaUkU3?=
 =?utf-8?B?T29BR2o0Z3Q2RGtSSmc3dFRyNk1CTnF2Y3F0MHhHNG1MVGlqMHQ1T1FlUnJJ?=
 =?utf-8?B?cDM1T2ZQMCttNkpYcmlCTmZ4TWYwZElaWFd2amc4T245VnZUbzQvVm5GT25w?=
 =?utf-8?B?dmczVVJBeHM4QWY4dDI1NVllZnoyUWwzTFpHNk5qMjNWekd2R1IybVpvRmRr?=
 =?utf-8?B?SjdiTFNwZ0lCWDJsakdsNWp5OVJTOStrR0RpajJ5amluZE1QcFBvekJJUURS?=
 =?utf-8?B?YW8zT2RFUFdXWitYL0tXdGZoMk1LODUwM2RSeGxjVVJLL1R0V2MvT0VOOStB?=
 =?utf-8?B?WThGWm1JQlE2bHB4Y3V5RVAvRDUvQm80OHU3eTAvbG9Bd05XcjJnb3RuL201?=
 =?utf-8?B?NzhEdE0yMkllQ1Z1a2RySFVrWnM5dG1XOEJXN3dYcEdHTVVaZ2pBQnpGYlVu?=
 =?utf-8?B?SCsrUlhWWGxzQ2ZsL01DcWYzc0RyZjdMWVRjeUhETmtNY3I4SWhnbTV5WUx6?=
 =?utf-8?B?emlzRGgxWXdOcDRLUHR1SENzWHlHUUJMcUttKzhTMDBsNEVsNkIwdzM2Qis1?=
 =?utf-8?B?QlhDanYvTHQwOWoxd2Z2Z3JEeVdEOGhqT3NHbnNJYUIzdURxaFlhS09HS0lw?=
 =?utf-8?B?SVhnRVhFTXFHUHhnVmlLMU92aTk0eDVVQ3FjbyswVFFZLzI0dnhaWlhUWTFN?=
 =?utf-8?B?aUR1SjVOdmxOaUhZMVlKcEZqNVdDcnFuNnRnT2xXeVRpUHJLRysra0lQVnhl?=
 =?utf-8?B?dCtKVzVIT2tLMTZ4M2UzelFGVFR4OWpUdGVzNUtMc3JoY1FKYkFQelZZOHIz?=
 =?utf-8?B?VFBqdzRUY0ZHRHFuNTY1UFBQMWQwQlhRNHZXampTTmZ6cGdHZ1BhWW4zckNM?=
 =?utf-8?B?by8rcHBIY0hLU2UwZ1g5VTVmUGhUMWdpeU1OL0dpVVR2YStyUVJBU1l4ZWpz?=
 =?utf-8?B?aFJudml6RHg0R0VjamorQ3ZQWnU4b0ZNdFhvVHFvYlFLR3pVdUVHR0U5VWF1?=
 =?utf-8?B?bnA0MUp0Ni9iZlU3VVJ0Z0J4dUwyU2pNalhWaVBWZVNBbUM2NnQ5TWZuS3h0?=
 =?utf-8?B?Wi85dkUvbzN4L1lUL3RnUlZyVmJZZUFqU1o4Q0d2WnBkbUt4NlB0cFp3S0o2?=
 =?utf-8?B?R2RsS1FOT3pZMWF3M1I5NXBLQnlYNkR0a0kxWldNSmh6bjdCRFE2Uk9nbXV3?=
 =?utf-8?B?MURwZlJyRVJQMVFPV0FqWk83TVJLWU10QkpwT01UalhTR0FKSGZoY3k3TGpX?=
 =?utf-8?B?ZzJPNlJjVzRYS3k5OVJsWHEwc2NaQU4rL1lETjFGS1ZuZ0cwRy9YY2xHcDVt?=
 =?utf-8?B?b05DOC9mWEFXNk1KRDJyN1FmWFZ3dEFUWlE1V1gvWFhHY2ZhTW5MSUlvQkZk?=
 =?utf-8?B?bzlybXRMdWlaNmJOTHowUmtLMTF2akNXYWlrQzllYXNnaHM4NnhESDBnZkh4?=
 =?utf-8?B?dlRNYzE3c1ZYZ1pLbGNnVmMzOThOdWlSV2Z0ektKcmZIWGxDUkxVNDJ6RTdt?=
 =?utf-8?B?Ly9WMC9FOE5Wc1ZOQW9PbC9CNkF4VjlOSklwN3FLQVVKWm1QanZ2WmFVeW11?=
 =?utf-8?B?bU8yamZyVXdHdldVLzA1ODAwQ2hqVGFWcEsvOFBRMFdMaHJmOGRDcVJDbWRC?=
 =?utf-8?B?UzRQMkVFRFViUHpYS3RvVnNKRDh1cGt2MWJ1d0FIdUtHVHlCV2V6RmhRcWxt?=
 =?utf-8?B?Y2oxUjBJMi96dkphZXF5Sm9EbWM3Zm9tT1Y1WFBXNXRKd3oxRm42dFdxMmpW?=
 =?utf-8?B?di9pWjEvZFI3OFIyU0NyZWtUWFFBUkV3UW5pYWdEbzVYNkxnYVVpcDJsUmFU?=
 =?utf-8?B?UEE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	R4ykQ5G4WywLgc/x5CbDf7vf7TRaUo4mqbmUGhbcLfHqJ8IKgYJFXqQ7mrEZIjecuFNporRipYMMabI6YnN4DHb+sxkArfxfjjS3eerO8DF0u0o/i8ba5oRTK/3vCxFFwBj6zHpqccbXvRC6KECBH9P9hJQbtvbpVR7hMvQ5UwGM9K41Li6x42KKI6tF4NVpSi4gB+B2KYxdt0MsDeQ6tFwLe67UGVU3ZMbBvC2QmqNQOhc0TECp4b19UxmXIR2oNKuHwJbCns9D2G6T8VSVb9YDE789dJpPqq8C8xu2XGdRJM52pQQ9GCwE0DEarJ94irvnyphcW1AR6miHTI75XE0KwmCwnxTl+6QieJ0Wy93cdbFni1+AQsc1uyFqDQilqOlssuun9vE3AWSyWZAftrQi6zVFAtvTZ6TaCCGTe1/5Q7Q4gr23lPXRNDup2adehs7XTB4qmSabOFWTB3BgrbGgO26xKfM52aEb+llSgNnwvMU1ppP7Pt1s/T25o3QoWFaaDMBxuo8Dl7+UkM+aC3Et7xiXVaMRQwXE7qZkG6cClkG5U0ykUhEYtxsAz+kA+ukRlrOWiio0xIEpdfEQebvisBb+Do6jUJLrT7yKFJE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ddb000c-9db7-4088-c16a-08dc2994de90
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 17:30:54.8010
 (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: N9inySIzxZnWg1Z/OB9z5rG4qpvurRc378suNPrzSZqST3vBSwIWg6271DLdAtlQ7iIkk/mkVialRb2gpvJATA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR10MB6069
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-09_15,2024-02-08_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0 mlxscore=0
 adultscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402090126
X-Proofpoint-ORIG-GUID: ta7eDbqBZwonRBI9aVv5lTZEGpux3bb7
X-Proofpoint-GUID: ta7eDbqBZwonRBI9aVv5lTZEGpux3bb7


>> +void xfs_get_atomic_write_attr(
>> +	struct xfs_inode *ip,
>> +	unsigned int *unit_min,
>> +	unsigned int *unit_max)
>> +{
>> +	xfs_extlen_t		extsz = xfs_get_extsz(ip);
>> +	struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
>> +	struct block_device	*bdev = target->bt_bdev;
>> +	unsigned int		awu_min, awu_max, align;
>> +	struct request_queue	*q = bdev->bd_queue;
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +
>> +	/*
>> +	 * Convert to multiples of the BLOCKSIZE (as we support a minimum
>> +	 * atomic write unit of BLOCKSIZE).
>> +	 */
>> +	awu_min = queue_atomic_write_unit_min_bytes(q);
>> +	awu_max = queue_atomic_write_unit_max_bytes(q);
>> +
>> +	awu_min &= ~mp->m_blockmask;
>> +	awu_max &= ~mp->m_blockmask;
> 
> I don't understand why we try to round down the awu_max to blocks size
> here and not just have an explicit check of (awu_max < blocksize).
We have later check for !awu_max:

if (!awu_max || !xfs_inode_atomicwrites(ip) || !align ||
..

So what we are doing is ensuring that the awu_max which the device 
reports is at least FS block size. If it is not, then we cannot support 
atomic writes.

Indeed, my NVMe drive reports awu_max  = 4K. So for XFS configured for 
64K block size, we will say that we don't support atomic writes.

> 
> I think the issue with changing the awu_max is that we are using awu_max
> to also indirectly reflect the alignment so as to ensure we don't cross
> atomic boundaries set by the hw (eg we check uint_max % atomic alignment
> == 0 in scsi). So once we change the awu_max, there's a chance that even
> if an atomic write aligns to the new awu_max it still doesn't have the
> right alignment and fails.

All these values should be powers-of-2, so rounding down should not 
affect whether we would not cross an atomic write boundary.

> 
> It works right now since eveything is power of 2 but it should cause
> issues incase we decide to remove that limitation. 

Sure, but that is a fundamental principle of this atomic write support. 
Not having powers-of-2 requirement for atomic writes will affect many 
things.

> Anyways, I think
> this implicit behavior of things working since eveything is a power of 2
> should atleast be documented in a comment, so these things are
> immediately clear.
> 
>> +
>> +	align = XFS_FSB_TO_B(mp, extsz);
>> +
>> +	if (!awu_max || !xfs_inode_atomicwrites(ip) || !align ||
>> +	    !is_power_of_2(align)) {
> 
> Correct me if I'm wrong but here as well, the is_power_of_2(align) is
> esentially checking if the align % uinit_max == 0 (or vice versa if
> unit_max is greater) 

yes

>so that an allocation of extsize will always align
> nicely as needed by the device.
>

I'm trying to keep things simple now.

In theory we could allow, say, align == 12 FSB, and then could say 
awu_max = 4.

The same goes for atomic write boundary in NVMe. Currently we say that 
it needs to be a power-of-2. However, it really just needs to be a 
multiple of awu_max. So if some HW did report a !power-of-2 atomic write 
boundary, we could reduce awu_max reported until to fits the power-of-2 
rule and also is cleanly divisible into atomic write boundary. But that 
is just not what HW will report (I expect). We live in a power-of-2 data 
granularity world.

> So maybe we should use the % expression explicitly so that the intention
> is immediately clear.

As mentioned, I wanted to keep it simple. In addition, it's a bit of a 
mess for the FS block allocator to work with odd sizes, like 12. And it 
does not suit RAID stripe alignment, which works in powers-of-2.

> 
>> +		*unit_min = 0;
>> +		*unit_max = 0;
>> +	} else {
>> +		if (awu_min)
>> +			*unit_min = min(awu_min, align);
> 
> How will the min() here work? If awu_min is the minumum set by the
> device, how can statx be allowed to advertise something smaller than
> that?
The idea is that is that if the awu_min reported by the device is less 
than the FS block size, then we report awu_min = FS block size. We 
already know that awu_max => FS block size, since we got this far, so 
saying that awu_min = FS block size is ok.

Otherwise it is the minimum of alignment and awu_min. I suppose that 
does not make much sense, and we should just always require awu_min = FS 
block size.

> 
> If I understand correctly, right now the way we set awu_min in scsi and
> nvme, the follwoing should usually be true for a sane device:
> 
>   awu_min <= blocks size of fs <= align
> 
>   so the min() anyways becomes redundant, but if we do assume that there
>   might be some weird devices with awu_min absurdly large (SCSI with
>   high atomic granularity) we still can't actually advertise a min
>   smaller than that of the device, or am I missing something here?

As above, I might just ensure that we can do awu_min = FS block size and 
not deal with crazy devices.

Thanks,
John