From: Eric Biggers <ebiggers@kernel.org>
To: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: linux-f2fs-devel@lists.sourceforge.net, Chao Yu <chao@kernel.org>,
linux-fsdevel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] f2fs: remove broken support for allocating DIO writes
Date: Fri, 30 Jul 2021 18:18:11 -0700 [thread overview]
Message-ID: <YQSk06ss5czNQ2bB@sol.localdomain> (raw)
In-Reply-To: <YQSh95wKJB+ax1VC@google.com>
On Fri, Jul 30, 2021 at 06:05:59PM -0700, Jaegeuk Kim wrote:
> On 07/30, Eric Biggers wrote:
> > On Fri, Jul 30, 2021 at 03:12:15PM -0700, Jaegeuk Kim wrote:
> > > On 07/30, Eric Biggers wrote:
> > > > On Tue, Jul 27, 2021 at 06:51:54PM -0700, Eric Biggers wrote:
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > >
> > > > > Currently, non-overwrite DIO writes are fundamentally unsafe on f2fs as
> > > > > they require preallocating blocks, but f2fs doesn't support unwritten
> > > > > blocks and therefore has to preallocate the blocks as regular blocks.
> > > > > f2fs has no way to reliably roll back such preallocations, so as a
> > >
> > > Hmm, I'm still wondering why this becomes a problem. And, do we really need
> > > to roll back the preallocated blocks?
> > >
> > > > > result, f2fs will leak uninitialized blocks to users if a DIO write
> > > > > doesn't fully complete. This can be easily reproduced by issuing a DIO
> > > > > write that will fail due to misalignment, e.g.:
> > >
> > > If there's any error, truncating blocks having NEW_ADDR could address this?
> > >
> >
> > My understanding is that the "NEW_ADDR" block address in f2fs means that space
> > was reserved for the block, but not allocated in any particular place yet.
> > Buffered writes reserve blocks in this way, but DIO writes cannot because DIO by
> > definition has to directly write to a specific on-disk location. Therefore DIO
> > writes require that the blocks be preallocated for real.
>
> Sorry, checking back the DIO flow, we do allocate real block addresses if DIO
> has holes.
>
> f2fs_preallocate_blocks
> -> f2fs_map_blocks(F2FS_GET_BLOCK_PRE_DIO)
> -> __allocate_data_block()
> -> f2fs_allocate_data_block() gets a free LBA
>
> Then, back to your concern, do we need to truncate blocks beyond i_size, if we
> meet any failure?
That isn't enough because an allocating write is not necessarily an extending
write; it may be filling holes. Also to be power-fail safe, the preallocations
must not be committed to disk at all until the write has completed (maybe that's
already the case in f2fs, but it's not clear to me).
- Eric
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org>
To: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: linux-fsdevel@vger.kernel.org, stable@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove broken support for allocating DIO writes
Date: Fri, 30 Jul 2021 18:18:11 -0700 [thread overview]
Message-ID: <YQSk06ss5czNQ2bB@sol.localdomain> (raw)
In-Reply-To: <YQSh95wKJB+ax1VC@google.com>
On Fri, Jul 30, 2021 at 06:05:59PM -0700, Jaegeuk Kim wrote:
> On 07/30, Eric Biggers wrote:
> > On Fri, Jul 30, 2021 at 03:12:15PM -0700, Jaegeuk Kim wrote:
> > > On 07/30, Eric Biggers wrote:
> > > > On Tue, Jul 27, 2021 at 06:51:54PM -0700, Eric Biggers wrote:
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > >
> > > > > Currently, non-overwrite DIO writes are fundamentally unsafe on f2fs as
> > > > > they require preallocating blocks, but f2fs doesn't support unwritten
> > > > > blocks and therefore has to preallocate the blocks as regular blocks.
> > > > > f2fs has no way to reliably roll back such preallocations, so as a
> > >
> > > Hmm, I'm still wondering why this becomes a problem. And, do we really need
> > > to roll back the preallocated blocks?
> > >
> > > > > result, f2fs will leak uninitialized blocks to users if a DIO write
> > > > > doesn't fully complete. This can be easily reproduced by issuing a DIO
> > > > > write that will fail due to misalignment, e.g.:
> > >
> > > If there's any error, truncating blocks having NEW_ADDR could address this?
> > >
> >
> > My understanding is that the "NEW_ADDR" block address in f2fs means that space
> > was reserved for the block, but not allocated in any particular place yet.
> > Buffered writes reserve blocks in this way, but DIO writes cannot because DIO by
> > definition has to directly write to a specific on-disk location. Therefore DIO
> > writes require that the blocks be preallocated for real.
>
> Sorry, checking back the DIO flow, we do allocate real block addresses if DIO
> has holes.
>
> f2fs_preallocate_blocks
> -> f2fs_map_blocks(F2FS_GET_BLOCK_PRE_DIO)
> -> __allocate_data_block()
> -> f2fs_allocate_data_block() gets a free LBA
>
> Then, back to your concern, do we need to truncate blocks beyond i_size, if we
> meet any failure?
That isn't enough because an allocating write is not necessarily an extending
write; it may be filling holes. Also to be power-fail safe, the preallocations
must not be committed to disk at all until the write has completed (maybe that's
already the case in f2fs, but it's not clear to me).
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2021-07-31 1:18 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-28 1:51 [f2fs-dev] [PATCH] f2fs: remove broken support for allocating DIO writes Eric Biggers
2021-07-28 1:51 ` Eric Biggers
2021-07-30 19:17 ` [f2fs-dev] " Eric Biggers
2021-07-30 19:17 ` Eric Biggers
2021-07-30 22:12 ` [f2fs-dev] " Jaegeuk Kim
2021-07-30 22:12 ` Jaegeuk Kim
2021-07-30 22:19 ` Eric Biggers
2021-07-30 22:19 ` [f2fs-dev] " Eric Biggers
2021-07-31 1:05 ` Jaegeuk Kim
2021-07-31 1:05 ` [f2fs-dev] " Jaegeuk Kim
2021-07-31 1:18 ` Eric Biggers [this message]
2021-07-31 1:18 ` Eric Biggers
2021-07-31 2:46 ` Theodore Ts'o
2021-07-31 2:46 ` [f2fs-dev] " Theodore Ts'o
2021-08-02 4:39 ` Eric Biggers
2021-08-02 4:39 ` Eric Biggers
2021-08-02 9:00 ` Chao Yu
2021-08-02 9:00 ` [f2fs-dev] " Chao Yu
2021-08-02 18:23 ` Jaegeuk Kim
2021-08-02 18:23 ` Jaegeuk Kim
2021-08-03 1:19 ` [f2fs-dev] " Chao Yu
2021-08-03 1:19 ` Chao Yu
2021-08-03 1:34 ` [f2fs-dev] " Jaegeuk Kim
2021-08-03 1:34 ` Jaegeuk Kim
2021-08-17 2:03 ` [f2fs-dev] " Eric Biggers
2021-08-17 2:03 ` Eric Biggers
2021-08-17 5:42 ` [f2fs-dev] " Christoph Hellwig
2021-08-17 5:42 ` Christoph Hellwig
2021-08-17 18:57 ` Jaegeuk Kim
2021-08-17 18:57 ` [f2fs-dev] " Jaegeuk Kim
2021-08-17 20:27 ` Eric Biggers
2021-08-17 20:27 ` [f2fs-dev] " Eric Biggers
2021-08-17 21:33 ` Jaegeuk Kim
2021-08-17 21:33 ` Jaegeuk Kim
2021-08-18 0:06 ` [f2fs-dev] " Eric Biggers
2021-08-18 0:06 ` Eric Biggers
2021-08-20 9:35 ` Chao Yu
2021-08-20 9:35 ` [f2fs-dev] " Chao Yu
2021-08-20 18:11 ` Eric Biggers
2021-08-20 18:11 ` Eric Biggers
2021-08-20 22:01 ` [f2fs-dev] " Chao Yu
2021-08-20 22:01 ` Chao Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YQSk06ss5czNQ2bB@sol.localdomain \
--to=ebiggers@kernel.org \
--cc=chao@kernel.org \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.