Received-SPF: pass (google.com: domain of linux-kernel+bounces-153672-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <363ee90b-00a4-45f9-91a3-663a8cdf077c@oracle.com>
Date: Mon, 22 Apr 2024 17:02:27 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC 5/7] fs: iomap: buffered atomic write support
To: Matthew Wilcox <willy@infradead.org>
Cc: axboe@kernel.dk, brauner@kernel.org, djwong@kernel.org,
        viro@zeniv.linux.org.uk, jack@suse.cz, akpm@linux-foundation.org,
        dchinner@redhat.com, tytso@mit.edu, hch@lst.de,
        martin.petersen@oracle.com, nilay@linux.ibm.com, ritesh.list@gmail.com,
        mcgrof@kernel.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        ojaswin@linux.ibm.com, p.raghav@samsung.com, jbongio@google.com,
        okiselev@amazon.com
References: <20240422143923.3927601-1-john.g.garry@oracle.com>
 <20240422143923.3927601-6-john.g.garry@oracle.com>
 <ZiZ8XGZz46D3PRKr@casper.infradead.org>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZiZ8XGZz46D3PRKr@casper.infradead.org>
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?TGU2allwYm1LY3UvVk94TklOeElhaDhHUzY0WVc1VXc3cEVGelVKNEFEaEcz?=
 =?utf-8?B?RlI2MGppcm1xSGp4MzdSSENvNGVGRGUrcDVDbTJ6anZic2NTWCtud01XY0VC?=
 =?utf-8?B?d2J2R0huUU5Ddkt6THlkdnJRTzVVODd2R0NiUzJnNDhCM3ZtNTZyZlJERWxj?=
 =?utf-8?B?QktwK3NKTVBjcWgwcGE0RmVFNFdUU2lNUkhxL0RJQmxuQytzSk85SFFtNVlu?=
 =?utf-8?B?aWgvVUF2cXpmWUJRZW44WXB2UUNSeDUvZDE1WnNOSXZ2OTdoSGdZMkcvSWdt?=
 =?utf-8?B?c29MYU5iVUFjTlh2bFRuTVdPVU5mTEpUeUFRS2kzMFlwYU5md1QyY0ZObjcv?=
 =?utf-8?B?dWc3SEVpWTBrQzh0SFk4cUxpQjZuVGRkdlNZMWd4NktFZ05EZUd6YzZqazVk?=
 =?utf-8?B?bksvS1hIeWN2OGZ3YlZ0R24xUGtZSEF3MTFkckNqRWFvUXUxM2MvUFJ6Mzlu?=
 =?utf-8?B?QWFQZUpGZE5aY2pGWXNEK2pzOUxoN0NqbFRISTFEamhURjgwTmlLWWVsYXBD?=
 =?utf-8?B?WlM4ays4cDlpbm81enZQalA3RjRTcHp6em42aUtKVU5OK0ExV082UWV0VEQw?=
 =?utf-8?B?SDdKSWo2cVR4VGN6UkZ3cUpkdWYrUlFlZU9GNGtxay9zNi9RV2dnRmI3UHRp?=
 =?utf-8?B?b3lySHo4UWZvTHVDSjZvTUhhdzFraWF3aU5IalFwRkVIZkhCVTIxcWhOVjR1?=
 =?utf-8?B?UXNhK1RTL0U5OG00aUh5eHVQeFZ2R1lYTlc3N3Jwb3BRWkplcmo5R3dCZU1S?=
 =?utf-8?B?aEppTmlKOThrSkF3OHhLK3NoNER1Yzg2cTZ2VXVaSFliVXhMem1WSlVOaHh5?=
 =?utf-8?B?OXpGc2dlRWQrVzJlZWxPeFFSbWNvK1hFRmpaQXIyVCtmMW1Gc1kzNGtjbWtu?=
 =?utf-8?B?bzRZb1UwdnNSQ1BVNXA0MlF1aUtFRFpqb053bTBBUXI3eGFnU1pxRHVtVkY3?=
 =?utf-8?B?ajVWR1pRM2tncXg3Q0VPR01MMzRwVzBRQTJjSjdXSzdEOTY2aTBhYmYyMWFN?=
 =?utf-8?B?VjJoY2dkaTMranZNd0p3YitZUUJGYnhIYVV4c29DaG5obGNKa3RpeWoyVGVl?=
 =?utf-8?B?STRhWWhIQk9FMlZyYWR1NFl5bkZLL2c0LzJEK3VETDZUMS9WN1YxNlNLcHlB?=
 =?utf-8?B?VUszMTV2Z29weHBQLzRwMnVHWGhYR1pLQ25aSnNZM3ovNUYrUUdMcWI3bDM0?=
 =?utf-8?B?Q2ZFMWVwUUY4SHpDa1M0dEY1TTNTVStLblpmOUxoajNPRlZhZ005ZmpNejgv?=
 =?utf-8?B?V2RxME5DZkxEdnVoNVRJOFJZS0xWWEtNeE0xNmhRcXNvRGZoNFFDQ2RBNHF4?=
 =?utf-8?B?YUNod25sL0VEODEyUFFxaml2WmdUc3RIeFBwWDZGa3hWZ2RBbVdYNkFkeFJP?=
 =?utf-8?B?ZHBicEZIWmZPdE1BcVF6R2d4MUkvMTVPb20zY0pSdVJjNGVEQUxWVHM2eHF5?=
 =?utf-8?B?Rkk2RjIvejdrYzNnSFN6TXNxYlJhZDZScUw1d3M2L1g5NU1HV2Q1UjBYQ0hn?=
 =?utf-8?B?RGEwbzROTllLRWJDbEExTy9xSUQrS3NIeWZEWW1aMldpOWp1SHAyTXVzZWVD?=
 =?utf-8?B?Rlk5cVRHMkF3YjdqSnNvZ2Z3Zk01V0hWbzN3aXhxNUJHTjN3Y1FUME9xVmh3?=
 =?utf-8?B?aTJmb05FSkU2Vnhla0FZSHFNYkpjMVpVY2pYUzhNSy9aSk1qb3M4RWhxTWRO?=
 =?utf-8?B?RkJQUzhQazdqKzY2Q3F4RFB3eEpMeDJ5QTNmR0pLL1JwNW8rT3NTQ3VkbEg2?=
 =?utf-8?B?NUVrMFNpWC8wd2RmS0NhaDVtSkpTMVYzTytpVGRNY25pcmhuWXdORzhsdVI3?=
 =?utf-8?B?bFNBMjRwYzdUaGRKekhkOFh5RTN0blRCejB1MERLOEQ3NXZTVGV5ckNkdXdI?=
 =?utf-8?B?M1dMZittc0xLd24vcUY5VldqL1VNbThOY0ZFeW1rM1JuQ2tadjVUMFp0V1Rz?=
 =?utf-8?B?aW9ES1Z5cXZYSXFmOVFuZk5neG8za2xueERHYVR2cG9jdldRV2pGMUJVZWpG?=
 =?utf-8?B?YUFNeW5GRTBMckpoWFBjeHluRmdoK0VrdU9BZmZvY25ETmQwajFodHpaUnJk?=
 =?utf-8?B?TmFuQkMveHNOOFRRdCtoUngxeDlQNFUzdUdsMWpjYk1sYXVvSlNmWGxNMFVZ?=
 =?utf-8?B?ZkJGQkJyK0pYQlpuOXdMMG5raExOZXVKK05xQUlKdGptbXJEQzVEUHB3U2oy?=
 =?utf-8?B?QXc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	MrtbGJZ+zcjAQt5DhcZj/MzAoTBiqo0AeU1f8EYiHM5DPFMxFBoBL/9S78MAxFFdoTtKiCExPG+Bp+/oAFB0OIDhxBqrEjADhyMG9IMvVP0S5TyS9X54mxbRoHGZ7uJo7qDbjCNLyHbrvEcJpwjxv1MiteTLCUbzMAiWnxM9UE3kjYE2r1X2iCsbXvyMsEahy8NM7FPBi6sPt+AjLAwwk929VcDKTIg+v5eSrsxZNUq3LbHtq/dZA2jdm5ZC8uSKT7olLeo5xoEekV7ui1hZzGV2VSJjnL8uXbU4RO9ndZhTd0jsgXeo05+hfy88dM3GjfoEkxSpQd64P0WYIovouzD1LHC8M0jeECFDfTs4UZ94yTtwh6MIZ0u1Od3s7pbByTOdDP26o22rQzZksC+PPyJ3e/weti874sqaMWpKAuG/KnGUdzsCDSohTPsdxYLgTicBQ5w8Muc1WGjhHexC+GUrlFuiKC8Ldc3CzkxUCSrJ/ty8IvdsH+jf+CqbJ0xBIZb4uLOPVVwQ9lsOr73XM5m8CuiMuOZCSwK5Mj+ChranlqRKdT45MzBxPS92zks+KtlGVpK+WGUPxidfmbuGree6Qg4zU2w/OdWmI+fa94s=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 11fc5f66-beb3-4383-1a39-08dc62e59e96
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2024 16:02:32.9599
 (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: C1Z8SXYSmlaGozNb4xEav/BsWYihprxodnqXnLYK2JjdD1XuFtjNXnvh3f/8bPrif6kYAzi+WG/R0retAVfGLg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB6666
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-22_09,2024-04-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 mlxscore=0 phishscore=0 spamscore=0 malwarescore=0 adultscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404220067
X-Proofpoint-ORIG-GUID: jVnHYxT69A2H2blz4gtJXHUk3RBribhP
X-Proofpoint-GUID: jVnHYxT69A2H2blz4gtJXHUk3RBribhP

On 22/04/2024 16:03, Matthew Wilcox wrote:
> On Mon, Apr 22, 2024 at 02:39:21PM +0000, John Garry wrote:
>> Add special handling of PG_atomic flag to iomap buffered write path.
>>
>> To flag an iomap iter for an atomic write, set IOMAP_ATOMIC.
>>
>> For a folio associated with a write which has IOMAP_ATOMIC set, set
>> PG_atomic.
>>
>> Otherwise, when IOMAP_ATOMIC is unset, clear PG_atomic.
>>
>> This means that for an "atomic" folio which has not been written back, it
>> loses it "atomicity". So if userspace issues a write with RWF_ATOMIC set
>> and another write with RWF_ATOMIC unset and which fully or partially
>> overwrites that same region as the first write, that folio is not written
>> back atomically. For such a scenario to occur, it would be considered a
>> userspace usage error.
>>
>> To ensure that a buffered atomic write is written back atomically when
>> the write syscall returns, RWF_SYNC or similar needs to be used (in
>> conjunction with RWF_ATOMIC).
>>
>> As a safety check, when getting a folio for an atomic write in
>> iomap_get_folio(), ensure that the length matches the inode mapping folio
>> order-limit.
>>
>> Only a single BIO should ever be submitted for an atomic write. So modify
>> iomap_add_to_ioend() to ensure that we don't try to write back an atomic
>> folio as part of a larger mixed-atomicity BIO.
>>
>> In iomap_alloc_ioend(), handle an atomic write by setting REQ_ATOMIC for
>> the allocated BIO.
>>
>> When a folio is written back, again clear PG_atomic, as it is no longer
>> required. I assume it will not be needlessly written back a second time...
> 
> I'm not taking a position on the mechanism yet; need to think about it
> some more.  But there's a hole here I also don't have a solution to,
> so we can all start thinking about it.
> 
> In iomap_write_iter(), we call copy_folio_from_iter_atomic().  Through no
> fault of the application, if the range crosses a page boundary, we might
> partially copy the bytes from the first page, then take a page fault on
> the second page, hence doing a short write into the folio.  And there's
> nothing preventing writeback from writing back a partially copied folio.
> 
> Now, if it's not dirty, then it can't be written back.  So if we're
> doing an atomic write, we could clear the dirty bit after calling
> iomap_write_begin() (given the usage scenarios we've discussed, it should
> always be clear ...)
> > We need to prevent the "fall back to a short copy" logic in
> iomap_write_iter() as well.  But then we also need to make sure we don't
> get stuck in a loop, so maybe go three times around, and if it's still
> not readable as a chunk, -EFAULT?
This idea sounds reasonable. So at what stage would the dirty flag be 
set? Would it be only when all bytes are copied successfully as a single 
chunk?

FWIW, we do have somewhat equivalent handling in direct IO path, being 
that if the iomap iter loops more than once such that we will need to 
create > 1 bio in the DIO bio submission handler, then we -EINVAL as 
something has gone wrong. But that's not so relevant here.

Thanks,
John