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: <20200928194103.244692-1-mfo@canonical.com> <20200928194103.244692-2-mfo@canonical.com>
 <2A963475-D65D-4E58-9EDD-93D6784934B4@dilger.ca>
In-Reply-To: <2A963475-D65D-4E58-9EDD-93D6784934B4@dilger.ca>
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
Date:   Wed, 30 Sep 2020 18:36:28 -0300
Message-ID: <CAO9xwp1AUy001ejrNBihodUdwAnQ9qceje+=Ko2nPuabDE9voQ@mail.gmail.com>
Subject: Re: [RFC PATCH v4 1/4] jbd2: introduce/export functions jbd2_journal_submit|finish_inode_data_buffers()
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
        dann frazier <dann.frazier@canonical.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Sep 28, 2020 at 11:24 PM Andreas Dilger <adilger@dilger.ca> wrote:
>
> On Sep 28, 2020, at 1:41 PM, Mauricio Faria de Oliveira <mfo@canonical.com> wrote:
> >
> > Export functions that implement the current behavior done
> > for an inode in journal_submit|finish_inode_data_buffers().
> >
> > No functional change.
> >
> > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Jan Kara <jack@suse.cz>
>
> A couple of minor cleanups below, but either way you could add:
>
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
>

Hey Andreas, thanks for reviewing!

These cleanups/style changes do look better -- applied to the two functions
in patch 1 (submit and finish), and another function in patch 4
(submit callback).

cheers,
Mauricio

> > +int jbd2_journal_finish_inode_data_buffers(struct jbd2_inode *jinode)
> > +{
> > +     struct address_space *mapping = jinode->i_vfs_inode->i_mapping;
> > +     loff_t dirty_start = jinode->i_dirty_start;
> > +     loff_t dirty_end = jinode->i_dirty_end;
> > +     int ret;
> > +
> > +     ret = filemap_fdatawait_range_keep_errors(mapping, dirty_start, dirty_end);
> > +     return ret;
> > +}
>
> (style) still prefer to wrap at 80 columns if possible.
> (style) there isn't any benefit to "dirty_start" and "dirty_end" as locals
> (style) there also isn't any benefit to "ret = ...; return ret"
>
> I thought it might be coded this way because the function is changed in a
> later patch in the series, but I couldn't find anything like that, so the
> shorter form is just as readable, IMHO:
>
> int jbd2_journal_finish_inode_data_buffers(struct jbd2_inode *jinode)
> {
>         struct address_space *mapping = jinode->i_vfs_inode->i_mapping;
>
>         return filemap_fdatawait_range_keep_errors(mapping,
>                                                    jinode->dirty_start,
>                                                    jinode->dirty_end);
> }
>
> Cheers, Andreas
>
>
>
>
>


-- 
Mauricio Faria de Oliveira