On Sun, Jul 25, 2021 at 06:50:51PM +0800, Chao Yu wrote:
> On 2021/7/16 22:39, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@xxxxxxxxxx>
> >
> > f2fs_write_begin() assumes that all blocks were preallocated by
> > default unless FI_NO_PREALLOC is explicitly set. This invites data
> > corruption, as there are cases in which not all blocks are preallocated.
> > Commit 47501f87c61a ("f2fs: preallocate DIO blocks when forcing
> > buffered_io") fixed one case, but there are others remaining.
>
> Could you please explain which cases we missed to handle previously?
> then I can check those related logic before and after the rework.
Any case where a buffered write happens while not all blocks were preallocated
but FI_NO_PREALLOC wasn't set. For example when ENOSPC was hit in the middle of
the preallocations for a direct write that will fall back to a buffered write,
e.g. due to f2fs_force_buffered_io() or page cache invalidation failure.
>
> > - /*
> > - * If force_buffere_io() is true, we have to allocate
> > - * blocks all the time, since f2fs_direct_IO will fall
> > - * back to buffered IO.
> > - */
> > - if (!f2fs_force_buffered_io(inode, iocb, from) &&
> > - f2fs_lfs_mode(F2FS_I_SB(inode)))
> > - goto write;
>
> We should keep this OPU DIO logic, otherwise, in lfs mode, write dio
> will always allocate two block addresses for each 4k append IO.
>
> I jsut test based on codes of last f2fs dev-test branch.
Yes, I had misread that due to the weird goto and misleading comment and
translated it into:
/* If it will be an in-place direct write, don't bother. */
if (dio && !f2fs_lfs_mode(sbi))
return 0;
It should be:
if (dio && f2fs_lfs_mode(sbi))
return 0;
Do you have a proper explanation for why preallocations shouldn't be done in
this case? Note that preallocations are still done for buffered writes, which
may be out-of-place as well; how are those different?
- Eric
