Re: [PATCH v2 08/14] fs: xfs: iomap: Sub-extent zeroing

John Garry
2024-03-07T12:57:26Z

>>
>> if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock)))
>> return xfs_alert_fsblock_zero(ip, imap);
>> @@ -134,6 +135,8 @@ xfs_bmbt_to_iomap(
>>
>> iomap->validity_cookie = sequence_cookie;
>> iomap->folio_ops = &xfs_iomap_folio_ops;
>> + if (extsz > 1)
>> + iomap->extent_shift = ffs(extsz) - 1;
>
> iomap->extent_size = mp->m_bsize;
> if (xfs_inode_has_force_align(ip))
> iomap->extent_size *= ip->i_extsize;

ok, fine

>
>> @@ -563,11 +566,19 @@ xfs_iomap_write_unwritten(
>> xfs_fsize_t i_size;
>> uint resblks;
>> int error;
>> + xfs_extlen_t extsz = xfs_get_extsz(ip);
>>
>> trace_xfs_unwritten_convert(ip, offset, count);
>>
>> - offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> - count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
>> + if (extsz > 1) {
>> + xfs_extlen_t extsize_bytes = XFS_FSB_TO_B(mp, extsz);
>> +
>> + offset_fsb = XFS_B_TO_FSBT(mp, round_down(offset, extsize_bytes));
>> + count_fsb = XFS_B_TO_FSB(mp, round_up(offset + count, extsize_bytes));
>> + } else {
>> + offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> + count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
>> + }
>
> I don't think this is correct. We should only be converting the
> extent when the entire range has had data written to it. If we are
> doing unaligned writes, we end up running 3 separate unwritten
> conversion transactions - the leading zeroing, the data written and
> the trailing zeroing.

Then I missed that in the code.

For sub-FS block conversion, I thought that this was doing the complete
FS blocks conversion, including for the head and tail zeros. And now for
sub-extent writes, we would be similarly doing the full extent
conversion, including head and tail zeros.

>
> This will end up converting the entire range to written when the
> leading zeroing completes, exposing stale data until the data and
> trailing zeroing completes.

That would not be good.

>
> Concurrent reads (both DIO and buffered) can see this stale data
> while the write is in progress, leading to a mechanism where a user
> can issue sub-atomic write range IO and concurrent overlapping reads
> to read arbitrary stale data from the disk just before it is
> overwritten.
>
> I suspect the only way to fix this for sub-force-aligned DIo writes
> if for iomap_dio_bio_iter() to chain the zeroing and data bios so
> the entire range gets a single completion run on it instead of three
> separate sub-aligned extent IO completions. We only need to do this
> in the zeroing case - this is already the DIo slow path, so
> additional submission overhead is not an issue. It would, however,
> reduce completion overhead and latency, as we only need to run a
> single extent conversion instead of 3, so chaining the bios on
> aligned writes may well be a net win...
>

ok, I'll check that idea.

> Thoughts? Christoph probably needs to weigh in on this one...
>

ok

Cheers,
John