On Jun 17, 2018, at 9:22 PM, Theodore Ts'o <tytso@xxxxxxx> wrote: > > Use a separate journal transaction if it turns out that we need to > convert an inline file to use an data block. Otherwise we could end > up failing due to not having journal credits. The callpath is a bit tricky here, but it seems that this is evicting the inline data from the xattr, but only putting all of the data into the dirty page for delalloc write? That would imply that the inline data xattr removal could commit in one transaction, but the delalloc write could be lost if the following transaction fails (e.g. crash), losing the original data along with any new data written to the file. I think it is acceptable to lose the newly-written delalloc data if it wasn't sync'd before the crash, but we shouldn't lose data that might have been written at some earlier time. Am I missing something here? Cheers, Andreas > https://bugzilla.kernel.org/show_bug.cgi?id=200071 > > Signed-off-by: Theodore Ts'o <tytso@xxxxxxx> > --- > fs/ext4/ext4.h | 3 --- > fs/ext4/inline.c | 38 +------------------------------------- > fs/ext4/xattr.c | 19 ++----------------- > 3 files changed, 3 insertions(+), 57 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 856b6a54d82b..859d6433dcc1 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3013,9 +3013,6 @@ extern int ext4_inline_data_fiemap(struct inode *inode, > struct iomap; > extern int ext4_inline_data_iomap(struct inode *inode, struct iomap *iomap); > > -extern int ext4_try_to_evict_inline_data(handle_t *handle, > - struct inode *inode, > - int needed); > extern int ext4_inline_data_truncate(struct inode *inode, int *has_inline); > > extern int ext4_convert_inline_data(struct inode *inode); > diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c > index d79115d8d716..851bc552d849 100644 > --- a/fs/ext4/inline.c > +++ b/fs/ext4/inline.c > @@ -887,11 +887,11 @@ int ext4_da_write_inline_data_begin(struct address_space *mapping, > flags |= AOP_FLAG_NOFS; > > if (ret == -ENOSPC) { > + ext4_journal_stop(handle); > ret = ext4_da_convert_inline_data_to_extent(mapping, > inode, > flags, > fsdata); > - ext4_journal_stop(handle); > if (ret == -ENOSPC && > ext4_should_retry_alloc(inode->i_sb, &retries)) > goto retry_journal; > @@ -1891,42 +1891,6 @@ int ext4_inline_data_fiemap(struct inode *inode, > return (error < 0 ? error : 0); > } > > -/* > - * Called during xattr set, and if we can sparse space 'needed', > - * just create the extent tree evict the data to the outer block. > - * > - * We use jbd2 instead of page cache to move data to the 1st block > - * so that the whole transaction can be committed as a whole and > - * the data isn't lost because of the delayed page cache write. > - */ > -int ext4_try_to_evict_inline_data(handle_t *handle, > - struct inode *inode, > - int needed) > -{ > - int error; > - struct ext4_xattr_entry *entry; > - struct ext4_inode *raw_inode; > - struct ext4_iloc iloc; > - > - error = ext4_get_inode_loc(inode, &iloc); > - if (error) > - return error; > - > - raw_inode = ext4_raw_inode(&iloc); > - entry = (struct ext4_xattr_entry *)((void *)raw_inode + > - EXT4_I(inode)->i_inline_off); > - if (EXT4_XATTR_LEN(entry->e_name_len) + > - EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size)) < needed) { > - error = -ENOSPC; > - goto out; > - } > - > - error = ext4_convert_inline_data_nolock(handle, inode, &iloc); > -out: > - brelse(iloc.bh); > - return error; > -} > - > int ext4_inline_data_truncate(struct inode *inode, int *has_inline) > { > handle_t *handle; > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 72377b77fbd7..723df14f4084 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -2212,23 +2212,8 @@ int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode, > if (EXT4_I(inode)->i_extra_isize == 0) > return -ENOSPC; > error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */); > - if (error) { > - if (error == -ENOSPC && > - ext4_has_inline_data(inode)) { > - error = ext4_try_to_evict_inline_data(handle, inode, > - EXT4_XATTR_LEN(strlen(i->name) + > - EXT4_XATTR_SIZE(i->value_len))); > - if (error) > - return error; > - error = ext4_xattr_ibody_find(inode, i, is); > - if (error) > - return error; > - error = ext4_xattr_set_entry(i, s, handle, inode, > - false /* is_block */); > - } > - if (error) > - return error; > - } > + if (error) > + return error; > header = IHDR(inode, ext4_raw_inode(&is->iloc)); > if (!IS_LAST_ENTRY(s->first)) { > header->h_magic = cpu_to_le32(EXT4_XATTR_MAGIC); > -- > 2.18.0.rc0 > Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
