From: "Amir G." <amir73il@users.sourceforge.net>
To: Lukas Czerner <lczerner@redhat.com>
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu,
Amir Goldstein <amir73il@users.sf.net>,
Yongqiang Yang <xiaoqiangnk@gmail.com>
Subject: Re: [PATCH RFC 03/30] ext4: snapshot hooks - inside JBD hooks
Date: Mon, 6 Jun 2011 22:01:13 +0300 [thread overview]
Message-ID: <BANLkTin1PZ2iU-90TDUGCGdwOONGuqs93w@mail.gmail.com> (raw)
In-Reply-To: <alpine.LFD.2.00.1106061732000.3934@dhcp-27-109.brq.redhat.com>
On Mon, Jun 6, 2011 at 6:53 PM, Lukas Czerner <lczerner@redhat.com> wrote:
> On Mon, 9 May 2011, amir73il@users.sourceforge.net wrote:
>
>> From: Amir Goldstein <amir73il@users.sf.net>
>>
>> Before every metadata buffer write, the journal API is called,
>> namely, one of the ext4_journal_get_XXX_access() functions.
>> We use these journal hooks to call the snapshot API, namely
>> ext4_snapshot_get_XXX_access(), to COW the metadata buffer before
>> it is modified for the first time.
>>
>> Signed-off-by: Amir Goldstein <amir73il@users.sf.net>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>> fs/ext4/ext4_jbd2.c | 9 +++++++--
>> fs/ext4/ext4_jbd2.h | 15 +++++++++++----
>> fs/ext4/extents.c | 3 ++-
>> fs/ext4/inode.c | 22 +++++++++++++++-------
>> fs/ext4/move_extent.c | 3 ++-
>> 5 files changed, 37 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
>> index 560020d..833969b 100644
>> --- a/fs/ext4/ext4_jbd2.c
>> +++ b/fs/ext4/ext4_jbd2.c
>> @@ -23,13 +23,16 @@ int __ext4_journal_get_undo_access(const char *where, unsigned int line,
>> return err;
>> }
>>
>> -int __ext4_journal_get_write_access(const char *where, unsigned int line,
>> - handle_t *handle, struct buffer_head *bh)
>> +int __ext4_journal_get_write_access_inode(const char *where, unsigned int line,
>> + handle_t *handle, struct inode *inode,
>> + struct buffer_head *bh, int exclude)
>> {
>> int err = 0;
>>
>> if (ext4_handle_valid(handle)) {
>> err = jbd2_journal_get_write_access(handle, bh);
>> + if (!err && !exclude)
>> + err = ext4_snapshot_get_write_access(handle, inode, bh);
>
> Agh, this is not defined anywhere again. Actually it is defined only if
> snapshot is not configured in. It is quite painful to review when half
> of the code is missing really. And also something like this will break
> bisecting for all of us. Is not there really the other way ?
Oh, sorry for the pain, I was trying to make life easier for reviewers, but that
often results in the opposite outcome...
These 'core' patches are not meant for merging, just for initial review.
In the 'full' patches, ext4_snapshot_get_write_access() is defined for
both configurations.
>
> Also, could you document the new parameters ?
>
> Anyway:
> if (!err && !exclude && inode)
>
It's documented in the snapshot hooks that inode can be NULL, so in the 'full'
patches that should be clear. sorry :-/
I promise to post the 'full' series soon.
>> if (err)
>> ext4_journal_abort_handle(where, line, __func__, bh,
>> handle, err);
>> @@ -111,6 +114,8 @@ int __ext4_journal_get_create_access(const char *where, unsigned int line,
>>
>> if (ext4_handle_valid(handle)) {
>> err = jbd2_journal_get_create_access(handle, bh);
>> + if (!err)
>> + err = ext4_snapshot_get_create_access(handle, bh);
>> if (err)
>> ext4_journal_abort_handle(where, line, __func__,
>> bh, handle, err);
>> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
>> index 8ffffb1..75662f7 100644
>> --- a/fs/ext4/ext4_jbd2.h
>> +++ b/fs/ext4/ext4_jbd2.h
>> @@ -132,9 +132,9 @@ void ext4_journal_abort_handle(const char *caller, unsigned int line,
>> int __ext4_journal_get_undo_access(const char *where, unsigned int line,
>> handle_t *handle, struct buffer_head *bh);
>>
>> -int __ext4_journal_get_write_access(const char *where, unsigned int line,
>> - handle_t *handle, struct buffer_head *bh);
>> -
>> +int __ext4_journal_get_write_access_inode(const char *where, unsigned int line,
>> + handle_t *handle, struct inode *inode,
>> + struct buffer_head *bh, int exclude);
>> int __ext4_forget(const char *where, unsigned int line, handle_t *handle,
>> int is_metadata, struct inode *inode,
>> struct buffer_head *bh, ext4_fsblk_t blocknr);
>> @@ -151,8 +151,15 @@ int __ext4_handle_dirty_super(const char *where, unsigned int line,
>>
>> #define ext4_journal_get_undo_access(handle, bh) \
>> __ext4_journal_get_undo_access(__func__, __LINE__, (handle), (bh))
>> +#define ext4_journal_get_write_access_exclude(handle, bh) \
>> + __ext4_journal_get_write_access_inode(__func__, __LINE__, \
>> + (handle), NULL, (bh), 1)
>> #define ext4_journal_get_write_access(handle, bh) \
>> - __ext4_journal_get_write_access(__func__, __LINE__, (handle), (bh))
>> + __ext4_journal_get_write_access_inode(__func__, __LINE__, \
>> + (handle), NULL, (bh), 0)
>> +#define ext4_journal_get_write_access_inode(handle, inode, bh) \
>> + __ext4_journal_get_write_access_inode(__func__, __LINE__, \
>> + (handle), (inode), (bh), 0)
>
> Could you add some comments so everyone knows when to use the _exclude
> helper and when not?
Will do. nobody should use it except for exclude bitmap write access.
I should change the name to get_exclude_bitmap_access to make that
more clear.
>
>> #define ext4_forget(handle, is_metadata, inode, bh, block_nr) \
>> __ext4_forget(__func__, __LINE__, (handle), (is_metadata), (inode), \
>> (bh), (block_nr))
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 0c3ea93..c8cab3d 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -77,7 +77,8 @@ static int ext4_ext_get_access(handle_t *handle, struct inode *inode,
>> {
>> if (path->p_bh) {
>> /* path points to block */
>> - return ext4_journal_get_write_access(handle, path->p_bh);
>> + return ext4_journal_get_write_access_inode(handle,
>> + inode, path->p_bh);
>> }
>> /* path points to leaf/index in inode body */
>> /* we use in-core data, no need to protect them */
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index a597ff1..b848072 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -874,7 +874,8 @@ static int ext4_splice_branch(handle_t *handle, struct inode *inode,
>> */
>> if (where->bh) {
>> BUFFER_TRACE(where->bh, "get_write_access");
>> - err = ext4_journal_get_write_access(handle, where->bh);
>> + err = ext4_journal_get_write_access_inode(handle, inode,
>> + where->bh);
>> if (err)
>> goto err_out;
>> }
>> @@ -4172,7 +4173,8 @@ static int ext4_clear_blocks(handle_t *handle, struct inode *inode,
>> goto out_err;
>> if (bh) {
>> BUFFER_TRACE(bh, "retaking write access");
>> - err = ext4_journal_get_write_access(handle, bh);
>> + err = ext4_journal_get_write_access_inode(handle,
>> + inode, bh);
>> if (unlikely(err))
>> goto out_err;
>> }
>> @@ -4223,7 +4225,8 @@ static void ext4_free_data(handle_t *handle, struct inode *inode,
>>
>> if (this_bh) { /* For indirect block */
>> BUFFER_TRACE(this_bh, "get_write_access");
>> - err = ext4_journal_get_write_access(handle, this_bh);
>> + err = ext4_journal_get_write_access_inode(handle, inode,
>> + this_bh);
>> /* Important: if we can't update the indirect pointers
>> * to the blocks, we can't free them. */
>> if (err)
>> @@ -4386,8 +4389,8 @@ static void ext4_free_branches(handle_t *handle, struct inode *inode,
>> * pointed to by an indirect block: journal it
>> */
>> BUFFER_TRACE(parent_bh, "get_write_access");
>> - if (!ext4_journal_get_write_access(handle,
>> - parent_bh)){
>> + if (!ext4_journal_get_write_access_inode(
>> + handle, inode, parent_bh)){
>> *p = 0;
>> BUFFER_TRACE(parent_bh,
>> "call ext4_handle_dirty_metadata");
>> @@ -4759,9 +4762,14 @@ has_buffer:
>>
>> int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc)
>> {
>> - /* We have all inode data except xattrs in memory here. */
>> - return __ext4_get_inode_loc(inode, iloc,
>> + int in_mem = (!EXT4_SNAPSHOTS(inode->i_sb) &&
>> !ext4_test_inode_state(inode, EXT4_STATE_XATTR));
>> +
>> + /*
>> + * We have all inode's data except xattrs in memory here,
>> + * but we must always read-in the entire inode block for COW.
>> + */
>> + return __ext4_get_inode_loc(inode, iloc, in_mem);
>> }
>>
>> void ext4_set_inode_flags(struct inode *inode)
>> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
>> index b9f3e78..ad5409a 100644
>> --- a/fs/ext4/move_extent.c
>> +++ b/fs/ext4/move_extent.c
>> @@ -421,7 +421,8 @@ mext_insert_extents(handle_t *handle, struct inode *orig_inode,
>>
>> if (depth) {
>> /* Register to journal */
>> - ret = ext4_journal_get_write_access(handle, orig_path->p_bh);
>> + ret = ext4_journal_get_write_access_inode(handle,
>> + orig_inode, orig_path->p_bh);
>> if (ret)
>> return ret;
>> }
>>
>
> --
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2011-06-06 19:01 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-09 16:41 [PATCH RFC 00/30] Ext4 snapshots - core patches amir73il
2011-05-09 16:41 ` [PATCH RFC 01/30] ext4: EXT4 snapshots (Experimental) amir73il
2011-06-06 14:50 ` Lukas Czerner
2011-06-07 9:28 ` Amir G.
2011-06-07 10:42 ` Lukas Czerner
2011-06-07 13:20 ` Amir G.
2011-05-09 16:41 ` [PATCH RFC 02/30] ext4: snapshot debugging support amir73il
2011-06-06 15:08 ` Lukas Czerner
2011-06-07 9:59 ` Amir G.
2011-06-07 10:49 ` Lukas Czerner
2011-05-09 16:41 ` [PATCH RFC 03/30] ext4: snapshot hooks - inside JBD hooks amir73il
2011-06-06 15:53 ` Lukas Czerner
2011-06-06 16:08 ` Amir G.
2011-06-06 19:01 ` Amir G. [this message]
2011-05-09 16:41 ` [PATCH RFC 04/30] ext4: snapshot hooks - block bitmap access amir73il
2011-05-09 16:41 ` [PATCH RFC 05/30] ext4: snapshot hooks - delete blocks amir73il
2011-06-07 11:24 ` Lukas Czerner
2011-06-07 13:24 ` Amir G.
2011-06-07 13:32 ` Lukas Czerner
2011-05-09 16:41 ` [PATCH RFC 06/30] ext4: snapshot hooks - move data blocks amir73il
2011-05-09 16:41 ` [PATCH RFC 07/30] ext4: snapshot hooks - direct I/O amir73il
2011-05-09 16:41 ` [PATCH RFC 08/30] ext4: snapshot hooks - move extent file data blocks amir73il
2011-05-09 16:41 ` [PATCH RFC 09/30] ext4: snapshot file amir73il
2011-06-02 11:52 ` Amir G.
2011-05-09 16:41 ` [PATCH RFC 10/30] ext4: snapshot file - read through to block device amir73il
2011-05-09 16:41 ` [PATCH RFC 11/30] ext4: snapshot file - permissions amir73il
2011-05-09 16:41 ` [PATCH RFC 12/30] ext4: snapshot file - store on disk amir73il
2011-05-09 16:41 ` [PATCH RFC 13/30] ext4: snapshot file - increase maximum file size limit to 16TB amir73il
2011-06-02 11:47 ` Amir G.
2011-06-03 0:48 ` Ted Ts'o
2011-06-03 4:45 ` Amir G.
2011-05-09 16:41 ` [PATCH RFC 14/30] ext4: snapshot block operations amir73il
2011-05-09 16:41 ` [PATCH RFC 15/30] ext4: snapshot block operation - copy blocks to snapshot amir73il
2011-05-09 16:41 ` [PATCH RFC 16/30] ext4: snapshot block operation - move " amir73il
2011-05-09 16:41 ` [PATCH RFC 17/30] ext4: snapshot control amir73il
2011-05-09 16:41 ` [PATCH RFC 18/30] ext4: snapshot control - fix new snapshot amir73il
2011-05-09 16:41 ` [PATCH RFC 19/30] ext4: snapshot control - reserve disk space for snapshot amir73il
2011-05-09 16:41 ` [PATCH RFC 20/30] ext4: snapshot journaled - increase transaction credits amir73il
2011-05-09 16:41 ` [PATCH RFC 21/30] ext4: snapshot journaled - implement journal_release_buffer() amir73il
2011-05-09 16:41 ` [PATCH RFC 22/30] ext4: snapshot journaled - bypass to save credits amir73il
2011-05-09 16:41 ` [PATCH RFC 23/30] ext4: snapshot journaled - trace COW/buffer credits amir73il
2011-05-09 16:41 ` [PATCH RFC 24/30] ext4: snapshot list support amir73il
2011-05-09 16:41 ` [PATCH RFC 25/30] ext4: snapshot race conditions - concurrent COW operations amir73il
2011-05-09 16:41 ` [PATCH RFC 26/30] ext4: snapshot race conditions - tracked reads amir73il
2011-05-09 16:41 ` [PATCH RFC 27/30] ext4: snapshot exclude - the exclude bitmap amir73il
2011-05-09 16:41 ` [PATCH RFC 28/30] ext4: snapshot cleanup amir73il
2011-05-09 16:41 ` [PATCH RFC 29/30] ext4: snapshot cleanup - shrink deleted snapshots amir73il
2011-05-09 16:41 ` [PATCH RFC 30/30] ext4: snapshot rocompat - enable rw mount amir73il
2011-06-06 13:08 ` [PATCH RFC 00/30] Ext4 snapshots - core patches Lukas Czerner
2011-06-06 14:32 ` Amir G.
2011-06-06 15:31 ` Eric Sandeen
2011-06-06 16:05 ` Lukas Czerner
2011-06-06 20:40 ` Ted Ts'o
2011-06-07 13:59 ` Ric Wheeler
2011-06-07 15:37 ` Ted Ts'o
2011-06-06 16:33 ` Andreas Dilger
2011-06-06 16:42 ` Eric Sandeen
2011-06-06 19:58 ` Lukáš Czerner
2011-06-06 18:25 ` Amir G.
2011-06-06 20:55 ` Ted Ts'o
2011-06-07 5:17 ` Andreas Dilger
2011-06-07 5:58 ` Amir G.
2011-06-07 10:09 ` Lukas Czerner
2011-06-07 13:01 ` Amir G.
2011-06-07 13:50 ` Ric Wheeler
2011-06-07 14:39 ` Amir G.
2011-06-07 6:40 ` Amir G.
2011-06-07 15:26 ` Josef Bacik
2011-06-07 16:46 ` Amir G.
2011-06-07 16:54 ` Josef Bacik
2011-06-07 18:22 ` Amir G.
2011-06-07 17:14 ` Sunil Mushran
2011-06-07 17:30 ` Ted Ts'o
2011-06-07 17:54 ` Amir G.
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=BANLkTin1PZ2iU-90TDUGCGdwOONGuqs93w@mail.gmail.com \
--to=amir73il@users.sourceforge.net \
--cc=amir73il@users.sf.net \
--cc=lczerner@redhat.com \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=xiaoqiangnk@gmail.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).