On Tue, Aug 13, 2019 at 10:40:39AM +0800, Chao Yu wrote:
> Hi Eric,
>
> On 2019/8/13 6:58, Eric Biggers wrote:
> > Hi Chao,
> >
> > On Mon, Aug 12, 2019 at 08:25:33PM +0800, Chao Yu wrote:
> >> Hi Eric,
> >>
> >> On 2019/8/12 5:35, Eric Biggers wrote:
> >>> From: Eric Biggers <ebiggers@xxxxxxxxxx>
> >>>
> >>> When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called
> >>> from f2fs_write_merkle_tree_block(), skip truncating the file. i_size
> >>> is not meaningful in this case, and the truncation is handled by
> >>> f2fs_end_enable_verity() instead.
> >>>
> >>> Fixes: 60d7bf0f790f ("f2fs: add fs-verity support")
> >>> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> >>> ---
> >>> fs/f2fs/data.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 3f525f8a3a5fa..00b03fb87bd9b 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
> >>> struct inode *inode = mapping->host;
> >>> loff_t i_size = i_size_read(inode);
> >>>
> >>> - if (to > i_size) {
> >>
> >> Maybe adding one single line comment to mention that it's redundant/unnecessary
> >> truncation here is better, if I didn't misunderstand this.
> >>
> >> Thanks,
> >>
> >>> + if (to > i_size && !f2fs_verity_in_progress(inode)) {
> >>> down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>> down_write(&F2FS_I(inode)->i_mmap_sem);
> >>>
> >
> > Do you mean add a comment instead of the !f2fs_verity_in_progress() check, or in
> > addition to it? ->write_begin(), ->writepages(), and ->write_end() are all
>
> Sorry, I didn't make this very clear, I meant adding the comment in addition on
> above change.
>
> > supposed to ignore i_size when verity is in progress, so I don't think this
> > particular part should be different, even if technically it's still correct to
> > truncate twice. Also, ext4 needs this check in its ->write_begin() for locking
> > reasons; we should keep f2fs similar.
>
> Agreed.
>
> >
> > How about having both a comment and the check, like:
> >
> > /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
> > if (to > i_size && !f2fs_verity_in_progress(inode)) {
>
> The comment looks good to me. :)
>
> Thanks,
>
> >
> > - Eric
> > .
> >
>
Okay, this is what I applied:
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3f525f8a3a5f..54cad80acb7d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2476,7 +2476,8 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
struct inode *inode = mapping->host;
loff_t i_size = i_size_read(inode);
- if (to > i_size) {
+ /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
+ if (to > i_size && !f2fs_verity_in_progress(inode)) {
down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
down_write(&F2FS_I(inode)->i_mmap_sem);
|