On Thu 06-02-20 23:28:01, zhangyi (F) wrote:
> Thanks for the comments.
>
> On 2020/2/6 19:46, Jan Kara wrote:
> > On Mon 03-02-20 22:04:58, zhangyi (F) wrote:
> [..]
> >> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> >> index 6396fe70085b..a649cdd1c5e5 100644
> >> --- a/fs/jbd2/commit.c
> >> +++ b/fs/jbd2/commit.c
> >> @@ -987,10 +987,13 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> >> if (buffer_freed(bh) && !jh->b_next_transaction) {
> >> clear_buffer_freed(bh);
> >> clear_buffer_jbddirty(bh);
> >> - clear_buffer_mapped(bh);
> >> - clear_buffer_new(bh);
> >> - clear_buffer_req(bh);
> >> - bh->b_bdev = NULL;
> >> + if (buffer_unmap(bh)) {
> >> + clear_buffer_unmap(bh);
> >> + clear_buffer_mapped(bh);
> >> + clear_buffer_new(bh);
> >> + clear_buffer_req(bh);
> >> + bh->b_bdev = NULL;
> >> + }
> >
> > Any reason why you don't want to clear buffer_req and buffer_new flags for
> > all buffers as well? I agree that b_bdev setting and buffer_mapped need
> > special treatment.
> >
> IIUC, for the buffer coming from jbd2_journal_forget() is always 'block
> device backed' metadata buffer (not pretty sure), and for these metadata
Yes, it is.
> buffer, buffer_new flag will not be set. At the same time, since it's
> always mapped, so it's fine to keep the buffer_req flag even it's freed
> by the filesystem now, because it means the block device has committed
> this buffer, and it seems that it does not affect we reuse this buffer.
> Am I missing something ?
OK, you're right that buffer_new shouldn't be ever set for block backed
buffers and we don't care about buffer_req. So let's keep the split of bits
to clear as you did and just add a comment that for block device buffers it
is enough to clear buffer_jbddirty and buffer_freed, for file mapping
buffers (i.e., journalled data) we have to be more careful and clear more
bits.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
