| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAO9xwp3mvXbGSMwPag431P+nGuVud2FK7n-Bq12LYLqm8uNOug@mail.gmail.com>
Date: Tue, 18 Aug 2020 22:20:08 -0300
From: Mauricio Faria de Oliveira <mfo@...onical.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org,
dann frazier <dann.frazier@...onical.com>,
Mauricio Faria de Oliveira <mauricio.foliveira@...il.com>,
Jan Kara <jack@...e.com>
Subject: Re: [RFC PATCH v2 2/5] jbd2: introduce journal callbacks j_submit|finish_inode_data_buffers
Hi Jan,
Thanks for reviewing.
On Tue, Aug 18, 2020 at 11:52 AM Jan Kara <jack@...e.cz> wrote:
>
> On Sun 09-08-20 22:02:05, Mauricio Faria de Oliveira wrote:
> > Add the callbacks as opt-in to override the default behavior for
> > the transaction's inode list, instead of moving that code around.
> >
> > This is important as not only ext4 uses the inode list: ocfs2 too,
> > via jbd2_journal_inode_ranged_write(), and maybe out-of-tree code.
>
> I'd prefer if the callback is called unconditionally, jbd2 exports the
> callback that implements the current behavior and and both ext4 & ocfs2
> are adapted to use this callback. We don't care about out of tree code.
> That way things are cleaner long term...
Understood.
>
> > To opt-out of the default behavior (i.e., to do nothing), one has
> > to opt-in with a no-op function.
>
> Your Signed-off-by is missing for this patch.
Oh, I thought it wasn't needed in RFC patches.
Thanks for the suggestions below; they're more precise and descriptive.
I had a few questions in the cover letter, but in case you didn't have
the time, I'll just try harder on them; no worries.
Kind regards,
Mauricio
>
> > ---
> > fs/jbd2/commit.c | 21 ++++++++++++++++-----
> > include/linux/jbd2.h | 21 ++++++++++++++++++++-
> > 2 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > index 51f713089e35..b98d227b50d8 100644
> > --- a/fs/jbd2/commit.c
> > +++ b/fs/jbd2/commit.c
> > @@ -237,10 +237,14 @@ static int journal_submit_data_buffers(journal_t *journal,
> > * instead of writepages. Because writepages can do
> > * block allocation with delalloc. We need to write
> > * only allocated blocks here.
> > + * This can be overriden with a custom callback.
> > */
> > trace_jbd2_submit_inode_data(jinode->i_vfs_inode);
> > - err = journal_submit_inode_data_buffers(mapping, dirty_start,
> > - dirty_end);
> > + if (journal->j_submit_inode_data_buffers)
> > + err = journal->j_submit_inode_data_buffers(jinode);
> > + else
> > + err = journal_submit_inode_data_buffers(mapping,
> > + dirty_start, dirty_end);
> > if (!ret)
> > ret = err;
> > spin_lock(&journal->j_list_lock);
> > @@ -274,9 +278,16 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
> > continue;
> > jinode->i_flags |= JI_COMMIT_RUNNING;
> > spin_unlock(&journal->j_list_lock);
> > - err = filemap_fdatawait_range_keep_errors(
> > - jinode->i_vfs_inode->i_mapping, dirty_start,
> > - dirty_end);
> > + /*
> > + * Wait for the inode data buffers writeout.
> > + * This can be overriden with a custom callback.
> > + */
> > + if (journal->j_finish_inode_data_buffers)
> > + err = journal->j_finish_inode_data_buffers(jinode);
> > + else
> > + err = filemap_fdatawait_range_keep_errors(
> > + jinode->i_vfs_inode->i_mapping,
> > + dirty_start, dirty_end);
> > if (!ret)
> > ret = err;
> > spin_lock(&journal->j_list_lock);
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index d56128df2aff..24efe88eda1b 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -628,7 +628,8 @@ struct transaction_s
> > struct journal_head *t_shadow_list;
> >
> > /*
> > - * List of inodes whose data we've modified in data=ordered mode.
> > + * List of inodes whose data we've modified in data=ordered mode
> > + * or whose pages we should write-protect in data=journaled mode.
>
> I'd rather change the comment to generic "List of inodes associated with
> the transaction. E.g. ext4 uses this to track inodes in data=ordered and
> data=journal mode that need special handling on transaction commit.".
>
> > * [j_list_lock]
> > */
> > struct list_head t_inode_list;
> > @@ -1110,6 +1111,24 @@ struct journal_s
> > void (*j_commit_callback)(journal_t *,
> > transaction_t *);
> >
> > + /**
> > + * @j_submit_inode_data_buffers:
> > + *
> > + * This function is called before flushing metadata buffers.
> > + * This overrides the default behavior (writeout data buffers.)
> > + */
>
> I'd change the comment to:
> * This function is called for all inodes associated with the
> * committing transaction marked with JI_WRITE_DATA flag before we
> * start to write out the transaction to the journal.
>
> > + int (*j_submit_inode_data_buffers)
> > + (struct jbd2_inode *);
> > +
> > + /**
> > + * @j_finish_inode_data_buffers:
> > + *
> > + * This function is called after flushing metadata buffers.
> > + * This overrides the default behavior (wait writeout.)
> > + */
>
> And here:
> * This function is called for all inodes associated with the
> * committing transaction marked with JI_WAIT_DATA flag after we
> * we have written the transaction to the journal but before we
> * write out the commit block.
>
>
> > + int (*j_finish_inode_data_buffers)
> > + (struct jbd2_inode *);
> > +
> > /*
> > * Journal statistics
> > */
>
> Honza
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR
--
Mauricio Faria de Oliveira
Powered by blists - more mailing lists