| Message ID | 20240730113335.2365290-5-shikemeng@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix and cleanups to jbd2 | expand |
On Tue 30-07-24 19:33:32, Kemeng Shi wrote: > Remove kmap for page of b_frozen_data from jbd2_alloc() which always > provides an address from the direct kernel mapping. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> I don't think this is really a win. On majority of installations kmap is a noop anyway and for the remainder kmap_local() is cheap. And the readability of the code is just worse with this. Honza > --- > fs/jbd2/journal.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 312c7575b54f..9c1ffb0dc740 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -352,12 +352,13 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, > done_copy_out = 1; > new_folio = virt_to_folio(jh_in->b_frozen_data); > new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data); > + mapped_data = jh_in->b_frozen_data; > } else { > new_folio = bh_in->b_folio; > new_offset = offset_in_folio(new_folio, bh_in->b_data); > + mapped_data = kmap_local_folio(new_folio, new_offset); > } > > - mapped_data = kmap_local_folio(new_folio, new_offset); > /* > * Fire data frozen trigger if data already wasn't frozen. Do this > * before checking for escaping, as the trigger may modify the magic > @@ -373,7 +374,8 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, > */ > if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) > do_escape = 1; > - kunmap_local(mapped_data); > + if (!jh_in->b_frozen_data) > + kunmap_local(mapped_data); > > /* > * Do we need to do a data copy? > -- > 2.30.0 >
On Tue 30-07-24 15:32:48, Jan Kara wrote: > On Tue 30-07-24 19:33:32, Kemeng Shi wrote: > > Remove kmap for page of b_frozen_data from jbd2_alloc() which always > > provides an address from the direct kernel mapping. > > > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > > I don't think this is really a win. On majority of installations kmap is a > noop anyway and for the remainder kmap_local() is cheap. And the > readability of the code is just worse with this. Ah, the following patch actually improves the code flow so the end result looks fine. OK, feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Maybe mention in the changelog that following patch will further improve the function. Honza > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > > index 312c7575b54f..9c1ffb0dc740 100644 > > --- a/fs/jbd2/journal.c > > +++ b/fs/jbd2/journal.c > > @@ -352,12 +352,13 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, > > done_copy_out = 1; > > new_folio = virt_to_folio(jh_in->b_frozen_data); > > new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data); > > + mapped_data = jh_in->b_frozen_data; > > } else { > > new_folio = bh_in->b_folio; > > new_offset = offset_in_folio(new_folio, bh_in->b_data); > > + mapped_data = kmap_local_folio(new_folio, new_offset); > > } > > > > - mapped_data = kmap_local_folio(new_folio, new_offset); > > /* > > * Fire data frozen trigger if data already wasn't frozen. Do this > > * before checking for escaping, as the trigger may modify the magic > > @@ -373,7 +374,8 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, > > */ > > if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) > > do_escape = 1; > > - kunmap_local(mapped_data); > > + if (!jh_in->b_frozen_data) > > + kunmap_local(mapped_data); > > > > /* > > * Do we need to do a data copy? > > -- > > 2.30.0 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR >
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 312c7575b54f..9c1ffb0dc740 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -352,12 +352,13 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, done_copy_out = 1; new_folio = virt_to_folio(jh_in->b_frozen_data); new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data); + mapped_data = jh_in->b_frozen_data; } else { new_folio = bh_in->b_folio; new_offset = offset_in_folio(new_folio, bh_in->b_data); + mapped_data = kmap_local_folio(new_folio, new_offset); } - mapped_data = kmap_local_folio(new_folio, new_offset); /* * Fire data frozen trigger if data already wasn't frozen. Do this * before checking for escaping, as the trigger may modify the magic @@ -373,7 +374,8 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction, */ if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) do_escape = 1; - kunmap_local(mapped_data); + if (!jh_in->b_frozen_data) + kunmap_local(mapped_data); /* * Do we need to do a data copy?
Remove kmap for page of b_frozen_data from jbd2_alloc() which always provides an address from the direct kernel mapping. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/jbd2/journal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)