linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: "zhangyi (F)" <yi.zhang@huawei.com>
Cc: Jan Kara <jack@suse.cz>,
	tytso@mit.edu, linux-ext4@vger.kernel.org, luoshijie1@huawei.com,
	zhangxiaoxu5@huawei.com
Subject: Re: [PATCH 2/2] jbd2: do not clear the BH_Mapped flag when forgetting a metadata buffer
Date: Wed, 12 Feb 2020 09:47:40 +0100	[thread overview]
Message-ID: <20200212084740.GB25573@quack2.suse.cz> (raw)
In-Reply-To: <bc3e2187-b1a7-b21e-db9f-c8c01b97368f@huawei.com>

On Tue 11-02-20 14:51:10, zhangyi (F) wrote:
> On 2020/2/6 19:46, Jan Kara wrote:
> > On Mon 03-02-20 22:04:58, zhangyi (F) wrote:
> >> Commit 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from
> >> an older transaction") set the BH_Freed flag when forgetting a metadata
> >> buffer which belongs to the committing transaction, it indicate the
> >> committing process clear dirty bits when it is done with the buffer. But
> >> it also clear the BH_Mapped flag at the same time, which may trigger
> >> below NULL pointer oops when block_size < PAGE_SIZE.
> >>
> >> rmdir 1             kjournald2                 mkdir 2
> >>                     jbd2_journal_commit_transaction
> >> 		    commit transaction N
> >> jbd2_journal_forget
> >> set_buffer_freed(bh1)
> >>                     jbd2_journal_commit_transaction
> >>                      commit transaction N+1
> >>                      ...
> >>                      clear_buffer_mapped(bh1)
> >>                                                ext4_getblk(bh2 ummapped)
> >>                                                ...
> >>                                                grow_dev_page
> >>                                                 init_page_buffers
> >>                                                  bh1->b_private=NULL
> >>                                                  bh2->b_private=NULL
> >>                      jbd2_journal_put_journal_head(jh1)
> >>                       __journal_remove_journal_head(hb1)
> >> 		       jh1 is NULL and trigger oops
> >>
> >> *) Dir entry block bh1 and bh2 belongs to one page, and the bh2 has
> >>    already been unmapped.
> >>
> >> For the metadata buffer we forgetting, clear the dirty flags is enough,
> >> so this patch add BH_Unmap flag for the journal_unmap_buffer() case and
> >> keep the mapped flag for the metadata buffer.
> >>
> >> Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction")
> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> [..]
> > 
> > Also rather than introducing this new buffer_unmap bit, I'd use the fact
> > this special treatment is needed only for buffers coming from the block device
> > mapping. And we can check for that like:
> > 
> > 		/*
> > 		 * We can (and need to) unmap buffer only for normal mappings.
> > 		 * Block device buffers need to stay mapped all the time.
> > 		 * We need to be careful about the check because the page
> > 		 * mapping can get cleared under our hands.
> > 		 */
> > 		mapping = READ_ONCE(bh->b_page->mapping);
> > 		if (mapping && !sb_is_blkdev_sb(mapping->host->i_sb)) {
> > 			...
> > 		}
> 
> Think about it again, it may missing clearing of mapped flag if 'mapping'
> of journalled data page was cleared, and finally trigger exception if
> we reuse the buffer again. So I think it should be:
> 
> 		if (!(mapping && sb_is_blkdev_sb(mapping->host->i_sb))) {
> 			...
> 		}

Well, if b_page->mapping got cleared, it means the page got fully truncated
and in such case buffers can never be reused - the page and buffers will be
freed once we are done with them. So what you are concerned about cannot
happen. But you're right it is good to explain this in the comment.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply	other threads:[~2020-02-12  8:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-03 14:04 [PATCH 0/2] jbd2: fix an oops problem zhangyi (F)
2020-02-03 14:04 ` [PATCH 1/2] jbd2: move the clearing of b_modified flag to the journal_unmap_buffer() zhangyi (F)
2020-02-06 11:03   ` Jan Kara
2020-02-03 14:04 ` [PATCH 2/2] jbd2: do not clear the BH_Mapped flag when forgetting a metadata buffer zhangyi (F)
2020-02-06 11:46   ` Jan Kara
2020-02-06 15:28     ` zhangyi (F)
2020-02-12  8:45       ` Jan Kara
2020-02-11  6:51     ` zhangyi (F)
2020-02-12  8:47       ` Jan Kara [this message]
2020-02-12 13:14         ` zhangyi (F)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200212084740.GB25573@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=luoshijie1@huawei.com \
    --cc=tytso@mit.edu \
    --cc=yi.zhang@huawei.com \
    --cc=zhangxiaoxu5@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).