[Cluster-devel] [PATCH] gfs2: Allocate bufdata object before taking log lock

[Andreas Gruenbacher]

On Tue, Apr 6, 2021 at 7:09 PM Andreas Gruenbacher <agruenba at redhat.com> wrote:
> Before this patch, functions gfs2_trans_add_{meta,data} would take the
> log lock, then if they needed to allocate a new bufdata object, they
> would drop the lock and reacquire it.  This patch changes it so that if
> there's apparently no bufdata object, it allocates a new one before
> taking the log lock.  After taking the log lock, it checks for conflicts
> and takes measures to resolve the conflict.  This is for performance.
>
> I haven't verified if the page locking in gfs2_trans_add_meta,
> originally introduced in commit 18ec7d5c3f434 ("Make journaled data
> files identical to normal files on disk"), is actually still needed.

This patch is still causing locking issues, so we'll leave it out for now.

> Signed-off-by: Andreas Gruenbacher <agruenba at redhat.com>
> ---
>  fs/gfs2/trans.c | 46 +++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/fs/gfs2/trans.c b/fs/gfs2/trans.c
> index 63fec11ef2ce..c50de22d7cbf 100644
> --- a/fs/gfs2/trans.c
> +++ b/fs/gfs2/trans.c
> @@ -171,7 +171,6 @@ static struct gfs2_bufdata *gfs2_alloc_bufdata(struct gfs2_glock *gl,
>         INIT_LIST_HEAD(&bd->bd_list);
>         INIT_LIST_HEAD(&bd->bd_ail_st_list);
>         INIT_LIST_HEAD(&bd->bd_ail_gl_list);
> -       bh->b_private = bd;
>         return bd;
>  }
>
> @@ -193,24 +192,24 @@ void gfs2_trans_add_data(struct gfs2_glock *gl, struct buffer_head *bh)
>  {
>         struct gfs2_trans *tr = current->journal_info;
>         struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
> -       struct gfs2_bufdata *bd;
> +       struct gfs2_bufdata *bd = NULL;
>
>         lock_buffer(bh);
>         if (buffer_pinned(bh)) {
>                 set_bit(TR_TOUCHED, &tr->tr_flags);
>                 goto out;
>         }
> -       gfs2_log_lock(sdp);
> -       bd = bh->b_private;
> -       if (bd == NULL) {
> -               gfs2_log_unlock(sdp);
> +       if (!bh->b_private) {
>                 unlock_buffer(bh);
> -               if (bh->b_private == NULL)
> -                       bd = gfs2_alloc_bufdata(gl, bh);
> -               else
> -                       bd = bh->b_private;
> +               bd = gfs2_alloc_bufdata(gl, bh);
>                 lock_buffer(bh);
> -               gfs2_log_lock(sdp);
> +       }
> +       gfs2_log_lock(sdp);
> +       if (bh->b_private) {
> +               kfree(bd);
> +               bd = bh->b_private;
> +       } else {
> +               bh->b_private = bd;
>         }
>         gfs2_assert(sdp, bd->bd_gl == gl);
>         set_bit(TR_TOUCHED, &tr->tr_flags);
> @@ -230,7 +229,7 @@ void gfs2_trans_add_meta(struct gfs2_glock *gl, struct buffer_head *bh)
>  {
>
>         struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
> -       struct gfs2_bufdata *bd;
> +       struct gfs2_bufdata *bd = NULL;
>         struct gfs2_meta_header *mh;
>         struct gfs2_trans *tr = current->journal_info;
>         enum gfs2_freeze_state state = atomic_read(&sdp->sd_freeze_state);
> @@ -240,19 +239,24 @@ void gfs2_trans_add_meta(struct gfs2_glock *gl, struct buffer_head *bh)
>                 set_bit(TR_TOUCHED, &tr->tr_flags);
>                 goto out;
>         }
> -       gfs2_log_lock(sdp);
> -       bd = bh->b_private;
> -       if (bd == NULL) {
> -               gfs2_log_unlock(sdp);
> +       if (!bh->b_private) {
>                 unlock_buffer(bh);
> +               bd = gfs2_alloc_bufdata(gl, bh);
> +               lock_buffer(bh);
> +       }
> +       gfs2_log_lock(sdp);
> +       if (bh->b_private) {
> +               kfree(bd);
> +               bd = bh->b_private;
> +       } else {
>                 lock_page(bh->b_page);
> -               if (bh->b_private == NULL)
> -                       bd = gfs2_alloc_bufdata(gl, bh);
> -               else
> +               if (bh->b_private) {
> +                       kfree(bd);
>                         bd = bh->b_private;
> +               } else {
> +                       bh->b_private = bd;
> +               }
>                 unlock_page(bh->b_page);
> -               lock_buffer(bh);
> -               gfs2_log_lock(sdp);
>         }
>         gfs2_assert(sdp, bd->bd_gl == gl);
>         set_bit(TR_TOUCHED, &tr->tr_flags);
> --
> 2.26.3
>

Thanks,
Andreas