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@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;
>
> 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;
+
