Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201027132751.29858-1-jack@suse.cz>
In-Reply-To: <20201027132751.29858-1-jack@suse.cz>
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
Date:   Tue, 27 Oct 2020 13:10:39 -0300
Message-ID: <CAO9xwp0AtCLG77g6fWgu9un9XPD3d5U6ZtjWc3FRJrB8NK44SQ@mail.gmail.com>
Subject: Re: [PATCH] ext4: Fix mmap write protection for data=journal mode
To:     Jan Kara <jack@suse.cz>
Cc:     Ted Tso <tytso@mit.edu>, linux-ext4@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Jan,

On Tue, Oct 27, 2020 at 10:27 AM Jan Kara <jack@suse.cz> wrote:
>
> Commit afb585a97f81 "ext4: data=journal: write-protect pages on
> j_submit_inode_data_buffers()") added calls ext4_jbd2_inode_add_write()
> to track inode ranges whose mappings need to get write-protected during
> transaction commits. However the added calls use wrong start of a range
> (0 instead of page offset) and so write protection is not necessarily
> effective. Use correct range start to fix the problem.
>
> Fixes: afb585a97f81 ("ext4: data=journal: write-protect pages on j_submit_inode_data_buffers()")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Mauricio, I think this could be the reason for occasional test failures you
> were still seeing. Can you try whether this patch fixes those for you? Thanks!
>

Thanks! Nice catch. Sure, I'll give it a try and follow up.

Just as FYI, I've been working on debugging and instrumentation to
identify the corner cases
where the page has not been write protected; but got a slower pace due
to other work items.

cheers,
Mauricio

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 03c2253005f0..f4a599c6dcde 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1918,7 +1918,7 @@ static int __ext4_journalled_writepage(struct page *page,
>         }
>         if (ret == 0)
>                 ret = err;
> -       err = ext4_jbd2_inode_add_write(handle, inode, 0, len);
> +       err = ext4_jbd2_inode_add_write(handle, inode, page_offset(page), len);
>         if (ret == 0)
>                 ret = err;
>         EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
> @@ -6157,7 +6157,8 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>                         if (ext4_walk_page_buffers(handle, page_buffers(page),
>                                         0, len, NULL, write_end_fn))
>                                 goto out_error;
> -                       if (ext4_jbd2_inode_add_write(handle, inode, 0, len))
> +                       if (ext4_jbd2_inode_add_write(handle, inode,
> +                                                     page_offset(page), len))
>                                 goto out_error;
>                         ext4_set_inode_state(inode, EXT4_STATE_JDATA);
>                 } else {
> --
> 2.16.4
>


-- 
Mauricio Faria de Oliveira