When endio conversion of an unwritten extent fails (e.g. ENOSPC while trying to split the extent), ext4 falls back to zeroing out the unwritten portions of the extent and marking it as written. This is safe only when no other in-flight writes have valid data underneath other parts of the same unwritten extent, otherwise the zeroout would silently corrupt that data. Commit ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O") changed the dio path to submit I/O without pre-splitting extents, deferring the split to endio. This means multiple concurrent writes can share a single unwritten extent, and a failed split on one of them could zeroout data belonging to another. Buffered IO fixes this by counting such IOs on unwritten extents in i_conv_pending_ios and decrementing them after conversion. However, due to the way dio iomap is designed, it is not very straightforward for the FS layer to increment once per unwritten IO. Hence, to keep it simple use the existing inode->i_dio_count which is incremented/decremented once per dio. zeroout code can now use i_dio_count with i_conv_pending_ios to decide if zeroout is safe to do. The only thing missing is the mutual exclusion of i_dio_count increment vs the zeroout decision. To fix that, add ext4_conv_pending_barrier() in ext4_iomap_begin() which just rotates i_conv_pending_sem before returning an unwritten mapping for a write. This ensures that either a racing zeroout has finished before we issue an IO over unwritten extent, or zeroout starts after our dio is accounted for. This design is overly cautious in disabling zeroout (eg, we will disable it for all inflight dio, not just dio over unwritten extents) but it keeps the code simple. Fixes: ea96cb5c4ae3 ("ext4: don't split extent before submitting I/O") Signed-off-by: Ojaswin Mujoo --- fs/ext4/ext4.h | 2 ++ fs/ext4/extents.c | 9 ++++++++- fs/ext4/inode.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 44cd9f12d350..bf390b3b5606 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -4002,6 +4002,8 @@ extern void ext4_conv_pending_sum(struct inode *inode, int n); extern int ext4_conv_pending_inc_trylock(struct inode *inode); extern void ext4_conv_pending_dec(struct inode *inode); extern void ext4_conv_pending_sub(struct inode *inode, int n); +extern void ext4_conv_pending_barrier(struct inode *inode); +extern int ext4_conv_pending_barrier_trylock(struct inode *inode); extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num, struct ext4_ext_path *path); diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 96d8ad012b2a..e4e65f3f576d 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3333,10 +3333,17 @@ static bool ext4_conv_pending_zeroout_lock(struct inode *inode, int flags) pending = atomic_read(&ei->i_conv_pending_ios); WARN_ON_ONCE(pending < 0); - if (pending > thresh || pending < 0) { + if (pending < 0) { up_write(&ei->i_conv_pending_sem); return false; } + + pending += atomic_read(&inode->i_dio_count); + if (pending > thresh) { + up_write(&ei->i_conv_pending_sem); + return false; + } + return true; } diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 8b5cb37cdd6d..9d7d4da0071f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -692,9 +692,9 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n) * they might have data underneath which is pending endio conversion. Zeroing * out such an extent can cause silent corruption. * - * To fix this we track the IOs which are about to undergo conversion in - * ei->i_conv_pending_ios. The zeroout code then uses this counter to - * determine if it's safe to zeroout. + * To fix this we track the no. of buffered IOs which are about to undergo + * conversion in ei->i_conv_pending_ios. The zeroout code then uses this counter + * to determine if it's safe to zeroout. * * Design Notes: * @@ -715,6 +715,9 @@ static void _ext4_conv_pending_mod(struct inode *inode, int n) * don't see any heavy lock contention. Atomic counters help parallel * modifications to be serialized. * + * Note that dio does not use i_conv_pending_ios; instead we use i_dio_count to + * disable zeroout if there are any other inflight dios. + * * ** Tolerance for false positives ** * * We trade false positives for simplicity of implementation because having a @@ -756,6 +759,30 @@ int ext4_conv_pending_inc_trylock(struct inode *inode) return 0; } +/* + * Rotate i_conv_pending_sem. This guarantees mutual exclusion against a + * concurrent endio zeroout, which holds i_conv_pending_sem exclusive. This + * barrier ensures either the zeroout has completed before we return, or it will + * start after we have accounted for the io and will see it while zeroing out. + * This can be used for paths that might not be changing ei->i_conv_pending_ios + */ +void ext4_conv_pending_barrier(struct inode *inode) +{ + down_read(&EXT4_I(inode)->i_conv_pending_sem); + up_read(&EXT4_I(inode)->i_conv_pending_sem); +} + +/* + * Same as ext4_conv_pending_barrier() but doesn't sleep on i_conv_pending_sem. + */ +int ext4_conv_pending_barrier_trylock(struct inode *inode) +{ + if (!down_read_trylock(&EXT4_I(inode)->i_conv_pending_sem)) + return -EAGAIN; + up_read(&EXT4_I(inode)->i_conv_pending_sem); + return 0; +} + void ext4_conv_pending_dec(struct inode *inode) { _ext4_conv_pending_mod(inode, -1); @@ -4073,6 +4100,16 @@ int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, if (ret < 0) return ret; out: + if ((flags & IOMAP_WRITE) && (map.m_flags & EXT4_MAP_UNWRITTEN)) { + if (flags & IOMAP_NOWAIT) { + ret = ext4_conv_pending_barrier_trylock(inode); + if (ret) + return ret; + } else { + ext4_conv_pending_barrier(inode); + } + } + /* * Sometimes I/O to an encrypted file has to be broken up to guarantee * DUN contiguity. Handle this by limiting the length of the mapping -- 2.55.0