Message ID | 20190811213557.1970-4-ebiggers@kernel.org (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | fs-verity fixups | expand |
Hi Eric, On 2019/8/12 5:35, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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@google.com> > --- > 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); > >
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@google.com> > > > > 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@google.com> > > --- > > 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 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. 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)) { - Eric
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@google.com> >>> >>> 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@google.com> >>> --- >>> 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 > . >
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@google.com> > >>> > >>> 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@google.com> > >>> --- > >>> 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);
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) { + 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);