Message-ID: <CAD+ocby8+qHFUP1bi64gKyaJwg2muy8mRyRt3fqkxTWvJMVNSw@mail.gmail.com>
Date:   Tue, 1 Oct 2019 00:50:58 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2 02/12] jbd2: add fast commit fields to journal_s structure

Thanks, done in V3.


On Fri, Aug 9, 2019 at 12:48 PM Andreas Dilger <adilger@...ger.ca> wrote:
>
> On Aug 8, 2019, at 9:45 PM, Harshad Shirwadkar <harshadshirwadkar@...il.com> wrote:
> >
> > For fast commits, JBD2 as of now allocates a default of 128 blocks at
> > the end of the journalling area. Although JBD2 owns these blocks, it
> > doesn't control what exactly should be written in these blocks. It
> > just provides the right abstraction for making these blocks usable by
> > file systems. This patch adds necessary fields to manage these fast
> > commit blocks.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
>
> Contrary to the description, this patch doesn't really do anything beyond
> adding unused fields and constants to the header, and as such isn't really
> useful to land on its own since there is no way to see what the fields
> are used for.  In particular, I was going to say that JBD2_FAST_COMMIT_BLOCKS
> should only be reserved if the FAST_COMMIT feature is enabled (unlike what
> is written above, which implies that they are always reserved), otherwise
> it can impact filesystem performance even when the feature is not active.
>
> I'd recommend to merge these changes with the patch where the fields/constants
> are actually used.
>
> Cheers, Andreas
>
> > ---
> >
> > Changelog:
> >
> > V2: Added struct transaction_run_stats_s * argument to
> >    j_fc_commit_callback to collect stats
> > ---
> > include/linux/jbd2.h | 79 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index b7eed49b8ecd..9a750b732241 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -66,6 +66,7 @@ void __jbd2_debug(int level, const char *file, const char *func,
> > extern void *jbd2_alloc(size_t size, gfp_t flags);
> > extern void jbd2_free(void *ptr, size_t size);
> >
> > +#define JBD2_FAST_COMMIT_BLOCKS 128
> > #define JBD2_MIN_JOURNAL_BLOCKS 1024
> >
> > #ifdef __KERNEL__
> > @@ -918,6 +919,34 @@ struct journal_s
> >        */
> >       unsigned long           j_last;
> >
> > +     /**
> > +      * @j_first_fc:
> > +      *
> > +      * The block number of the first fast commit block in the journal
> > +      */
> > +     unsigned long           j_first_fc;
> > +
> > +     /**
> > +      * @j_current_fc:
> > +      *
> > +      * Journal fc block iterator
> > +      */
> > +     unsigned long           j_fc_off;
> > +
> > +     /**
> > +      * @j_last_fc:
> > +      *
> > +      * The block number of the last fast commit block in the journal
> > +      */
> > +     unsigned long           j_last_fc;
> > +
> > +     /**
> > +      * @j_do_full_commit:
> > +      *
> > +      * Force a full commit. If this flag is set JBD2 won't try fast commits
> > +      */
> > +     bool                    j_do_full_commit;
> > +
> >       /**
> >        * @j_dev: Device where we store the journal.
> >        */
> > @@ -987,6 +1016,15 @@ struct journal_s
> >        */
> >       tid_t                   j_transaction_sequence;
> >
> > +     /**
> > +      * @j_subtid:
> > +      *
> > +      * One plus the sequence number of the most recently committed fast
> > +      * commit. This represents the sub transaction ID for the next fast
> > +      * commit.
> > +      */
> > +     tid_t                   j_subtid;
> > +
> >       /**
> >        * @j_commit_sequence:
> >        *
> > @@ -1068,6 +1106,20 @@ struct journal_s
> >        */
> >       int                     j_wbufsize;
> >
> > +     /**
> > +      * @j_fc_wbuf:
> > +      *
> > +      * Array of bhs for fast commit transactions
> > +      */
> > +     struct buffer_head      **j_fc_wbuf;
> > +
> > +     /**
> > +      * @j_fc_wbufsize:
> > +      *
> > +      * Size of @j_fc_wbufsize array.
> > +      */
> > +     int                     j_fc_wbufsize;
> > +
> >       /**
> >        * @j_last_sync_writer:
> >        *
> > @@ -1167,6 +1219,33 @@ struct journal_s
> >        */
> >       struct lockdep_map      j_trans_commit_map;
> > #endif
> > +     /**
> > +      * @j_fc_commit_callback:
> > +      *
> > +      * File-system specific function that performs actual fast commit
> > +      * operation. Should return 0 if the fast commit was successful, in that
> > +      * case, JBD2 will just increment journal->j_subtid and move on. If it
> > +      * returns < 0, JBD2 will fall-back to full commit.
> > +      */
> > +     int (*j_fc_commit_callback)(struct journal_s *journal, tid_t tid,
> > +                                 tid_t subtid,
> > +                                 struct transaction_run_stats_s *stats);
> > +     /**
> > +      * @j_fc_replay_callback:
> > +      *
> > +      * File-system specific function that performs replay of a fast
> > +      * commit. JBD2 calls this function for each fast commit block found in
> > +      * the journal.
> > +      */
> > +     int (*j_fc_replay_callback)(struct journal_s *journal,
> > +                                 struct buffer_head *bh);
> > +     /**
> > +      * @j_fc_cleanup_callback:
> > +      *
> > +      * Clean-up after fast commit or full commit. JBD2 calls this function
> > +      * after every commit operation.
> > +      */
> > +     void (*j_fc_cleanup_callback)(struct journal_s *journal);
> > };
> >
> > #define jbd2_might_wait_for_commit(j) \
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
>
>
> Cheers, Andreas
>
>
>
>
>

Powered by blists - more mailing lists