Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <b8b0a9d7-88d2-45a9-877a-ecc5e0f1e645@oracle.com>
Date:   Wed, 13 Dec 2023 09:32:06 +0000
User-Agent: Mozilla Thunderbird
From:   John Garry <john.g.garry@oracle.com>
Subject: Re: [PATCH v2 00/16] block atomic writes
To:     Christoph Hellwig <hch@lst.de>
Cc:     axboe@kernel.dk, kbusch@kernel.org, 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,
        ming.lei@redhat.com, jaswin@linux.ibm.com, bvanassche@acm.org
References: <20231212110844.19698-1-john.g.garry@oracle.com>
 <20231212163246.GA24594@lst.de>
Content-Language: en-US
Organization: Oracle Corporation
In-Reply-To: <20231212163246.GA24594@lst.de>
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?eThvMEtkV21GekZ6aGNXc1ByWWZodGY1am5EaGRiRzlyaDgyRFEwNkVnKzVP?=
 =?utf-8?B?ZUc4bktqbVJ3QXdjVjErT1FsTm44c0pMMmtqc3UyWEtONjJNVXJEc1VZaUZm?=
 =?utf-8?B?YUhUbE9qUTFWeUQ5TklSeU5BWUcvdDZ1RHMvcmtoMDVzalZXblNMMDQ2WFZj?=
 =?utf-8?B?NCs4SlE0bFJKMmU1SUpuOXdMTEQyKy9Sb093dTRTR1IrMkwvdnprK2tOQlhB?=
 =?utf-8?B?eHFlNTFLUTRQbGRyYkFOWkpoeTVrOHBscGRhZDJaR2g2a3VGRmsvcmJTT0xp?=
 =?utf-8?B?NWNYMW5pMWs4UVlrRVFVMUpJUG5sMVV3WEd1VzBlWlFUbmNlcnlaZnh5U09O?=
 =?utf-8?B?V3VnTHFtK0RvdkhaYlhZUVF1Qld1TFpLb3V2V0djOWlHWkg2dUorWmRRQ0xN?=
 =?utf-8?B?aXlLSS9qZzZUSXNiQ05GR1FQc0ZxbDZ4VWM2TFhSaXI4SVM4RXRUQ2JuVmNO?=
 =?utf-8?B?bDk2NkZTaXErcTNuKy9pd1BqQXJaOEd1SHVpTjM4TGIrVFhwUnVIOWZObnc5?=
 =?utf-8?B?L3pFWVpIb0JFNlVrZDFESG02QUVCMzFnaS9lMlVEamlQa2hOajRkS3MxL0Nq?=
 =?utf-8?B?WTVpNmZqaDBTNWlPYjZURVVweHVHd1UwOVJsVDA5K3YxN3A0WUIrazRjU09Y?=
 =?utf-8?B?SDRZNDBTVGk0aHhwMXdFYzlSZHlIRHZqSm1oUGQzcWhpSUI5OERXWUl0ME9B?=
 =?utf-8?B?aGhhUHJqUDBxckFvb3JuYlpRWE9STU5XSmtMcnU4VVNMSjV6UEtEQW9PRWVt?=
 =?utf-8?B?ZW9uSmZkUm8yclJhajliR1AySU9CckFueHlkeklMSHJGK1d2c0YySGtQLzh2?=
 =?utf-8?B?c29tZ0lFUnNyd3I4ejlDQnJ5cUtXWXQzUGJGZXdJem5DVVJXaDNxckIwWTND?=
 =?utf-8?B?WFN2Wm9VdUVENlNDcFh3ZkJKL2Z6bkJYbHY5SWpjdXo5eGhGYXFIcDcvaHo1?=
 =?utf-8?B?eStkbnZmOGp3S29jZDZVUWkwQnNwSlhZSzNMTldNbnBacVBjTHBuZ1BQWUJE?=
 =?utf-8?B?ajl3ejNoUURtSEdVb1U0UVVubFFIczBsU0Rwc0h1dWorRldhYWQ3akVuOUQv?=
 =?utf-8?B?Mlo4Z1N4cC9hc0FvUXFzQy9mNWdpRXBPTVAvVm1WTmNNMmtBckdWSmtGdkRk?=
 =?utf-8?B?YmpNblZLTzl3RUJqL3gwY3dvT0YrMUI5WGpjUHNkTEpVTjE0T04waXdRVE42?=
 =?utf-8?B?S0lTU3ptYS9WNGhabkNzaFFRRkpYY2RhQThmVmpPTEFIcC9OUitqelN2Q2xt?=
 =?utf-8?B?VC9Jb2xwb1BrZGF5bTcwdnY0Snc0eUJ4ZGhJeDhodHhkRFpSSnd6Y1VKRUNX?=
 =?utf-8?B?ZWo4Wm1rNTR0ckpBYXR3UXY2c2NqV0g4SGhVWnU2My92UTh3REJxTzRhczNn?=
 =?utf-8?B?M0hmWGNXU09xYlArVWRLSm1OMHJmZnNNdGpTMXBGS1ZmNmRjTS9aT3kwYThU?=
 =?utf-8?B?K2NRQVJlR1ZJa0g3SGVOcVdvV21TQWJxTmYveUVzNEJGeG5ndGZtMkU3ZEE3?=
 =?utf-8?B?M1dMWElMcHRHM3FUSzY2R2tMcXN6V2Y2ZmcwTWpqRDAraWhGVFZ3QmFUTmJK?=
 =?utf-8?B?T3FOOFdRK1JOQVdic1pueTQxZ3lTY05GcnpZZ0lPenFqbUZ5d1kwZU9SODVT?=
 =?utf-8?B?NUlJVlRTcVNpOWRaV3o5d3pyMkhvUnJTOTdKdWdrTWJ1Zk1nVldqTmphaG5G?=
 =?utf-8?B?VmlBRVI1T3FxTGpRajRqYy96empMSE04bUZ1Vk9oQWFwR1M1YVA3VXZOOVZw?=
 =?utf-8?B?aUFjQUlxYmNqVWtxUTBIbUlZMnYyM2FuUjIvMjdRSWwrbTNrOE42cGZZMTBa?=
 =?utf-8?B?eHgzL0ZVNWFRRlV6c3MvVnZmRE5KTmNRS3JGeG1ydXVMemw5aVhJNEZvbU1w?=
 =?utf-8?B?aEMzTlprMnNLcFlIUm5zOFNBL0dod2puNjB0eW9FTTRwOXNGdXhwK1ltZ3Vk?=
 =?utf-8?B?RTU5eVlpbkFtMTU2aEFzYmQvZnlERC9VRVVuSUtIdFlobmtMLytteDJTNzgv?=
 =?utf-8?B?d2ZRS1FhcWpkT0pIVW9LNXFOb3c5YmIvZDZoSjZObmdOa1krYktSRDN0RmpK?=
 =?utf-8?B?M3djb3BVRVBuRjY5N1VOdXBOTDFnSGNHMTMzc1RFUG5xMzlRVlllOW1NWGlR?=
 =?utf-8?B?YUlZd2dPeldWSlVKOEoxSlEzbjJDYzhNSVFlSUtqam5uMmw5QWlWcmVZdzhk?=
 =?utf-8?B?dmc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: aXVMtJ285YLsW+SNQroFAZmNZ0AsVsWwr/KMFygTpn3sugdX9pJnDfUgxCNc0gImcGO4cIArkRAkdaou2Hv+6+nfBbPeDba8jjn0AsWW7/SrxhPWs5eiROtbI0IJffiHeKkAxHNsV9xscungWFFAnCiq1RmgNJ88O4UFheLg7S6HidPQluJ0dBoLYLBn+INFgR4pPzyNPVjMgXu2A+uboMJy9Y9F9lQBpiAaF1xjFjz05hI9n3yc/8djX6j4OzBtvOP0h36PRzaKi2OtFiBuzLScXI1tFJa+rM0T3VR1SufrOo+VJw3EUqGLUEkh9MdDoy/QYtqCrBwrdR+6UUe2GXHtuPKbl/nfKxi9nvZD5GyUbvwR0+76jV0t/Xvn4fsMYrJbZJ6WgkvAQu3h/mRQEYbYet9L6VTZTPTtr/YA64sVukPUnwQagpgIjzm/uOIM9vuFZHosO48Ssi88kL+mLqr9Gocb5GLKz7sP01fH/AFj1JpwuxBT22qNPtI5MIEr6kKuBT3XDZSd1dz+owStcxy03nfIm9Pfui35CTHbnBBlzQX3qs9vhgCwuPDErb8KG3sVwVZiBJHA7VWBf31Mz2n3kMYI/AbP80u+X3UBUS4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fc460972-9de3-438b-62e9-08dbfbbe6188
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 09:32:10.3760
 (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: eM1njiCqQVuVomyuxplsZVnS83dgywUqHyy2MP7pcQmkvWhCSRZj5YGb2hfhjsJR1ZzJwGYa+CHenWXSQ6eDnQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB7391
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-13_02,2023-12-12_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312130068
X-Proofpoint-ORIG-GUID: RaXrXy1qf9WHtTYwkwzKaHWgaqe7v1QE
X-Proofpoint-GUID: RaXrXy1qf9WHtTYwkwzKaHWgaqe7v1QE
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 morse.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 (morse.vger.email [0.0.0.0]); Wed, 13 Dec 2023 01:32:47 -0800 (PST)

On 12/12/2023 16:32, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 11:08:28AM +0000, John Garry wrote:
>> Two new fields are added to struct statx - atomic_write_unit_min and
>> atomic_write_unit_max. For each atomic individual write, the total length
>> of a write must be a between atomic_write_unit_min and
>> atomic_write_unit_max, inclusive, and a power-of-2. The write must also be
>> at a natural offset in the file wrt the write length.
>>
>> SCSI sd.c and scsi_debug and NVMe kernel support is added.
>>
>> Some open questions:
>> - How to make API extensible for when we have no HW support? In that case,
>>    we would prob not have to follow rule of power-of-2 length et al.
>>    As a possible solution, maybe we can say that atomic writes are
>>    supported for the file via statx, but not set unit_min and max values,
>>    and this means that writes need to be just FS block aligned there.
> I don't think the power of two length is much of a problem to be
> honest, and if we every want to lift it we can still do that easily
> by adding a new flag or limit.

ok, but it would be nice to have some idea on what that flag or limit 
change would be.

> 
> What I'm a lot more worried about is how to tell the file system that
> allocations are done right for these requirement.  There is no way
> a user can know that allocations in an existing file are properly
> aligned, so atomic writes will just fail on existing files.
> 
> I suspect we need an on-disk flag that forces allocations to be
> aligned to the atomic write limit, in some ways similar how the
> XFS rt flag works.  You'd need to set it on an empty file, and all
> allocations after that are guaranteed to be properly aligned.

Hmmm... so how is this different to the XFS forcealign feature?

For XFS, I thought that your idea was to always CoW new extents for 
misaligned extents or writes which spanned multiple extents.

> 
>> - For block layer, should atomic_write_unit_max be limited by
>>    max_sectors_kb? Currently it is not.
> Well.  It must be limited to max_hw_sectors to actually work.

Sure, as max_hw_sectors may also be limited by DMA controller max 
mapping size.

> max_sectors is a software limit below that, which with modern hardware
> is actually pretty silly and a real performance issue with todays
> workloads when people don't tweak it..

Right, so we should limit atomic write queue limits to max_hw_sectors. 
But people can still tweak max_sectors, and I am inclined to say that 
atomic_write_unit_max et al should be (dynamically) limited to 
max_sectors also.

> 
>> - How to improve requirement that iovecs are PAGE-aligned.
>>    There are 2x issues:
>>    a. We impose this rule to not split BIOs due to virt boundary for
>>       NVMe, but there virt boundary is 4K (and not PAGE size, so broken for
>>       16K/64K pages). Easy solution is to impose requirement that iovecs
>>       are 4K-aligned.
>>    b. We don't enforce this rule for virt boundary == 0, i.e. SCSI
> .. we require any device that wants to support atomic writes to not
> have that silly limit.  For NVMe that would require SGL support
> (and some driver changes I've been wanting to make for long where
> we always use SGLs for transfers larger than a single PRP if supported)

If we could avoid dealing with a virt boundary, then that would be nice.

Are there any patches yet for the change to always use SGLs for 
transfers larger than a single PRP?

On a related topic, I am not sure about how - or if we even should - 
enforce iovec PAGE-alignment or length; rather, the user could just be 
advised that iovecs must be PAGE-aligned and min PAGE length to achieve 
atomic_write_unit_max.

> 
> 
>> - Since debugging torn-writes due to unwanted kernel BIO splitting/merging
>>    would be horrible, should we add some kernel storage stack software
>>    integrity checks?
> Yes, I think we'll need asserts in the drivers.  At least for NVMe I
> will insist on them. 

Please see 16/16 in this series.

> For SCSI I think the device actually checks
> because the atomic writes are a different command anyway, or am I
> misunderstanding how SCSI works here?

Right, for WRITE ATOMIC (16) the target will reject a command when it 
does not respect the device atomic write limits.

Thanks,
John