On Mon, Sep 28, 2020 at 11:24 PM Andreas Dilger <adilger@xxxxxxxxx> wrote:
>
> On Sep 28, 2020, at 1:41 PM, Mauricio Faria de Oliveira <mfo@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Suggested-by: Jan Kara <jack@xxxxxxx>
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> A couple of minor cleanups below, but either way you could add:
>
> Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>
>
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
