Re: [PATCH 3/9] f2fs: rework write preallocations

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 07/26, Jaegeuk Kim wrote:
> On 07/25, Eric Biggers wrote:
> > 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@google.com>
> > > > 
> > > > 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;
> 
> Hmm, this addresses my 250 failure. And, I think the below commit can explain
> the case.

In addition to this, I got failure on generic/263, and the below change fixes
it. (I didn't take a look at deeply tho.)

--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -4344,8 +4344,13 @@ static int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *iter)
                        return ret;
        }

-       map.m_lblk = (pos >> inode->i_blkbits);
-       map.m_len = ((pos + count - 1) >> inode->i_blkbits) - map.m_lblk + 1;
+       map.m_lblk = F2FS_BLK_ALIGN(pos);
+       map.m_len = F2FS_BYTES_TO_BLK(pos + count);
+       if (map.m_len > map.m_lblk)
+               map.m_len -= map.m_lblk;
+       else
+               map.m_len = 0;
+