| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <99208409-87d4-4ed2-8294-665eafd60b47@oracle.com>
Date: Mon, 17 Mar 2025 14:25:30 +0000
From: John Garry <john.g.garry@...cle.com>
To: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>, brauner@...nel.org,
djwong@...nel.org, cem@...nel.org, dchinner@...hat.com, hch@....de
Cc: linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, ojaswin@...ux.ibm.com,
martin.petersen@...cle.com, tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v6 03/13] iomap: rework IOMAP atomic flags
On 17/03/2025 13:44, Ritesh Harjani (IBM) wrote:
>> if (flags & IOMAP_DAX)
>> iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
>> else
>> @@ -3467,7 +3470,7 @@ static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
>> return false;
>>
>> /* atomic writes are all-or-nothing */
>> - if (flags & IOMAP_ATOMIC_HW)
>> + if (flags & IOMAP_ATOMIC)
>> return false;
>>
> The changes in ext4 is mostly straight forward. Essentially for
> an IOMAP_ATOMIC write requests we are always setting IOMAP_F_ATOMIC_BIO in
> the ->iomap_begin() routine. This is done to inform the iomap that this
> write request needs to issue an atomic bio, so iomap then goes and sets
> REQ_ATOMIC flag in the bio.
Right
>
>
>> /* can only try again if we wrote nothing */
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index 9d72b99cb447..c28685fd3362 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -349,7 +349,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>> if (dio->flags & IOMAP_DIO_WRITE) {
>> bio_opf |= REQ_OP_WRITE;
>>
>> - if (iter->flags & IOMAP_ATOMIC_HW) {
>> + if (iomap->flags & IOMAP_F_ATOMIC_BIO) {
>> /*
>> * Ensure that the mapping covers the full write length,
>> * otherwise we will submit multiple BIOs, which is
>> @@ -677,10 +677,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>> iomi.flags |= IOMAP_OVERWRITE_ONLY;
>> }
>>
>> - if (dio_flags & IOMAP_DIO_ATOMIC_SW)
>> - iomi.flags |= IOMAP_ATOMIC_SW;
>> - else if (iocb->ki_flags & IOCB_ATOMIC)
>> - iomi.flags |= IOMAP_ATOMIC_HW;
>> + if (iocb->ki_flags & IOCB_ATOMIC)
>> + iomi.flags |= IOMAP_ATOMIC;
>>
>> /* for data sync or sync, we need sync completion processing */
>> if (iocb_is_dsync(iocb)) {
>> diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
>> index 69af89044ebd..9eab2c8ac3c5 100644
>> --- a/fs/iomap/trace.h
>> +++ b/fs/iomap/trace.h
>> @@ -99,7 +99,7 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
>> { IOMAP_FAULT, "FAULT" }, \
>> { IOMAP_DIRECT, "DIRECT" }, \
>> { IOMAP_NOWAIT, "NOWAIT" }, \
>> - { IOMAP_ATOMIC_HW, "ATOMIC_HW" }
>> + { IOMAP_ATOMIC, "ATOMIC" }
>>
>> #define IOMAP_F_FLAGS_STRINGS \
>> { IOMAP_F_NEW, "NEW" }, \
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 30e257f683bb..9a22ecd794eb 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -831,6 +831,9 @@ xfs_direct_write_iomap_begin(
>> if (offset + length > i_size_read(inode))
>> iomap_flags |= IOMAP_F_DIRTY;
>>
>> + if (flags & IOMAP_ATOMIC)
>> + iomap_flags |= IOMAP_F_ATOMIC_BIO;
>> +
>> /*
>> * COW writes may allocate delalloc space or convert unwritten COW
>> * extents, so we need to make sure to take the lock exclusively here.
>> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
>> index 9cd93530013c..51f4c13bd17a 100644
>> --- a/include/linux/iomap.h
>> +++ b/include/linux/iomap.h
>> @@ -60,6 +60,9 @@ struct vm_fault;
>> * IOMAP_F_ANON_WRITE indicates that (write) I/O does not have a target block
>> * assigned to it yet and the file system will do that in the bio submission
>> * handler, splitting the I/O as needed.
>> + *
>> + * IOMAP_F_ATOMIC_BIO indicates that (write) I/O needs to be issued as an
>> + * atomic bio, i.e. set REQ_ATOMIC.
>> */
>
> Maybe we can be more explicit here?
>
> IOMAP_F_ATOMIC_BIO flag indicates that write I/O must be issued as an
> atomic bio by setting the REQ_ATOMIC flag. Filesystems need to set this
> flag to inform iomap that the write I/O operation should be submitted as
> an atomic bio.
The comment for all these flags is that they should be set by the FS:
"Flags reported by the file system from iomap_begin"
So the second sentence seems to just repeat what is already said.
>
> This definition (or whatever you feel is the better version), should also
> go in Documentation/filesystems/iomap/design.rst
Yes, I need to update that again
>
>> #define IOMAP_F_NEW (1U << 0)
>> #define IOMAP_F_DIRTY (1U << 1)
>> @@ -73,6 +76,7 @@ struct vm_fault;
>> #define IOMAP_F_XATTR (1U << 5)
>> #define IOMAP_F_BOUNDARY (1U << 6)
>> #define IOMAP_F_ANON_WRITE (1U << 7)
>> +#define IOMAP_F_ATOMIC_BIO (1U << 8)
>>
>> /*
>> * Flags set by the core iomap code during operations:
>> @@ -189,9 +193,8 @@ struct iomap_folio_ops {
>> #else
>> #define IOMAP_DAX 0
>> #endif /* CONFIG_FS_DAX */
>> -#define IOMAP_ATOMIC_HW (1 << 9) /* HW-based torn-write protection */
>> +#define IOMAP_ATOMIC (1 << 9) /* torn-write protection */
>> #define IOMAP_DONTCACHE (1 << 10)
>> -#define IOMAP_ATOMIC_SW (1 << 11)/* SW-based torn-write protection */
> Now that we are killing separate IOMAP_ATOMIC_** names, we may would
> like to update the iomap design document as well. Otherwise it will
> carry use of IOMAP_ATOMIC_HW & IOMAP_ATOMIC_SW definitions. Instead we
> should only keep IOMAP_ATOMIC and update the design info there.
Yes, I will update it.
Thanks for the reminder.
John
Powered by blists - more mailing lists