| Message ID | 20201031200518.4178786-8-harshadshirwadkar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Ext4 fast commit fixes | expand |
On Sat 31-10-20 13:05:15, Harshad Shirwadkar wrote: > This patch adds a small number of misc fast commit fixes. Along with > functional fixes such as setting the right buffer flags, there also > typo fixes and comment additions. > > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> Again, please don't merge logically separate fixes into one commit. > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h > index 00dc668e052b..10855cd230c7 100644 > --- a/fs/ext4/ext4_jbd2.h > +++ b/fs/ext4/ext4_jbd2.h > @@ -422,9 +422,13 @@ static inline int ext4_journal_force_commit(journal_t *journal) > static inline int ext4_jbd2_inode_add_write(handle_t *handle, > struct inode *inode, loff_t start_byte, loff_t length) > { > - if (ext4_handle_valid(handle)) > + if (ext4_handle_valid(handle)) { > + ext4_fc_track_range(handle, inode, > + start_byte >> inode->i_sb->s_blocksize_bits, > + (start_byte + length) >> inode->i_sb->s_blocksize_bits); > return jbd2_journal_inode_ranged_write(handle, > EXT4_I(inode)->jinode, start_byte, length); > + } Why this change? A good changelog would tell me... I'm suspicious here because ext4_jbd2_inode_add_write() gets called only in data=ordered mode but fastcommit can run also in data=writeback mode... I suppose this is for the mmap coverage we were speaking about. Now that I'm speaking about it again maybe the ext4_fc_track_range() call in ext4_map_blocks() is actually enough? I mean once we allocate blocks for a range (either from page fault, write, or writeback of delalloc), they will become properly tracked in ext4_map_blocks() and that's all we need? But then I'm missing why we have so many ext4_fc_track_range() calls around the code... Can you please explain? Honza
On Tue, Nov 3, 2020 at 8:52 AM Jan Kara <jack@suse.cz> wrote: > > On Sat 31-10-20 13:05:15, Harshad Shirwadkar wrote: > > This patch adds a small number of misc fast commit fixes. Along with > > functional fixes such as setting the right buffer flags, there also > > typo fixes and comment additions. > > > > Suggested-by: Jan Kara <jack@suse.cz> > > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> > > Again, please don't merge logically separate fixes into one commit. Sure, I'll break this up. > > > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h > > index 00dc668e052b..10855cd230c7 100644 > > --- a/fs/ext4/ext4_jbd2.h > > +++ b/fs/ext4/ext4_jbd2.h > > @@ -422,9 +422,13 @@ static inline int ext4_journal_force_commit(journal_t *journal) > > static inline int ext4_jbd2_inode_add_write(handle_t *handle, > > struct inode *inode, loff_t start_byte, loff_t length) > > { > > - if (ext4_handle_valid(handle)) > > + if (ext4_handle_valid(handle)) { > > + ext4_fc_track_range(handle, inode, > > + start_byte >> inode->i_sb->s_blocksize_bits, > > + (start_byte + length) >> inode->i_sb->s_blocksize_bits); > > return jbd2_journal_inode_ranged_write(handle, > > EXT4_I(inode)->jinode, start_byte, length); > > + } > > Why this change? A good changelog would tell me... I'm suspicious here > because ext4_jbd2_inode_add_write() gets called only in data=ordered mode > but fastcommit can run also in data=writeback mode... > > I suppose this is for the mmap coverage we were speaking about. Now that > I'm speaking about it again maybe the ext4_fc_track_range() call in > ext4_map_blocks() is actually enough? I mean once we allocate blocks for a > range (either from page fault, write, or writeback of delalloc), they will > become properly tracked in ext4_map_blocks() and that's all we need? But > then I'm missing why we have so many ext4_fc_track_range() calls around the > code... Can you please explain? You are right. I was trying to handle the mmap case as we discussed on the previous patch set. But as I mentioned in one of the other patches in this series, I scanned all the callers of ext4_fc_track_range() and realized that there were a few redundant callers of this function. Ultimately, this function needs to get called only when blocks are added or removed from an inode. So the places where this call remains are - fallocate ops, ext4_map_blocks, truncate. Thanks, Harshad > > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h index 00dc668e052b..10855cd230c7 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -422,9 +422,13 @@ static inline int ext4_journal_force_commit(journal_t *journal) static inline int ext4_jbd2_inode_add_write(handle_t *handle, struct inode *inode, loff_t start_byte, loff_t length) { - if (ext4_handle_valid(handle)) + if (ext4_handle_valid(handle)) { + ext4_fc_track_range(handle, inode, + start_byte >> inode->i_sb->s_blocksize_bits, + (start_byte + length) >> inode->i_sb->s_blocksize_bits); return jbd2_journal_inode_ranged_write(handle, EXT4_I(inode)->jinode, start_byte, length); + } return 0; } diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 0f2543220d1d..b7b1fe6dbb24 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -83,7 +83,7 @@ * * Atomicity of commits * -------------------- - * In order to gaurantee atomicity during the commit operation, fast commit + * In order to guarantee atomicity during the commit operation, fast commit * uses "EXT4_FC_TAG_TAIL" tag that marks a fast commit as complete. Tail * tag contains CRC of the contents and TID of the transaction after which * this fast commit should be applied. Recovery code replays fast commit @@ -531,10 +531,11 @@ static void ext4_fc_submit_bh(struct super_block *sb) int write_flags = REQ_SYNC; struct buffer_head *bh = EXT4_SB(sb)->s_fc_bh; + /* TODO: REQ_FUA | REQ_PREFLUSH is unnecessarily expensive. */ if (test_opt(sb, BARRIER)) write_flags |= REQ_FUA | REQ_PREFLUSH; lock_buffer(bh); - clear_buffer_dirty(bh); + set_buffer_dirty(bh); set_buffer_uptodate(bh); bh->b_end_io = ext4_end_buffer_io_sync; submit_bh(REQ_OP_WRITE, write_flags, bh); diff --git a/fs/ext4/file.c b/fs/ext4/file.c index d85412d12e3a..80ad5ccc0288 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -761,7 +761,6 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) if (!daxdev_mapping_supported(vma, dax_dev)) return -EOPNOTSUPP; - ext4_fc_start_update(inode); file_accessed(file); if (IS_DAX(file_inode(file))) { vma->vm_ops = &ext4_dax_vm_ops; @@ -769,7 +768,6 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) } else { vma->vm_ops = &ext4_file_vm_ops; } - ext4_fc_stop_update(inode); return 0; }
This patch adds a small number of misc fast commit fixes. Along with functional fixes such as setting the right buffer flags, there also typo fixes and comment additions. Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> --- fs/ext4/ext4_jbd2.h | 6 +++++- fs/ext4/fast_commit.c | 5 +++-- fs/ext4/file.c | 2 -- 3 files changed, 8 insertions(+), 5 deletions(-)