[Cluster-devel] [GFS2 PATCH v2] GFS2: Submit all ordered writes in bulk, wait after that

[Andreas Gruenbacher]

On 18 June 2018 at 19:28, Bob Peterson <rpeterso at redhat.com> wrote:
> Hi,
>
> This is a new and improved version of the patch set I posted on 14 May.
> ---
> Before this patch, the ordered_write function would submit all
> the ordered writes with filemap_fdatawrite, which waited for each
> one to complete before moving on to the next. This patch allows it
> to submit them all, then wait for them after they're submitted.
> It also implements Andreas's suggestion to call the filemap
> functions directly rather than a more complicated ordered_wait.
> It also does the sorting outside the spin_lock.

This patch is bad and needs to be removed from for-next, unfortunately.

> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> ---
>  fs/gfs2/log.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 0248835625f1..c7c4549ba1c9 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -22,6 +22,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/writeback.h>
>  #include <linux/list_sort.h>
> +#include <linux/fs.h>
>
>  #include "gfs2.h"
>  #include "incore.h"
> @@ -534,22 +535,32 @@ static void gfs2_ordered_write(struct gfs2_sbd *sdp)
>  {
>         struct gfs2_inode *ip;
>         LIST_HEAD(written);
> +       struct writeback_control wbc = {
> +               .sync_mode = WB_SYNC_NONE,
> +               .nr_to_write = LONG_MAX,
> +               .range_start = 0,
> +               .range_end = LLONG_MAX,
> +       };
>
>         spin_lock(&sdp->sd_ordered_lock);
> -       list_sort(NULL, &sdp->sd_log_le_ordered, &ip_cmp);
>         while (!list_empty(&sdp->sd_log_le_ordered)) {
>                 ip = list_entry(sdp->sd_log_le_ordered.next, struct gfs2_inode, i_ordered);
> -               if (ip->i_inode.i_mapping->nrpages == 0) {
> +               if (ip->i_inode.i_mapping->nrpages)
> +                       list_move(&ip->i_ordered, &written);
> +               else {
>                         test_and_clear_bit(GIF_ORDERED, &ip->i_flags);
>                         list_del(&ip->i_ordered);
> -                       continue;
>                 }
> -               list_move(&ip->i_ordered, &written);
> -               spin_unlock(&sdp->sd_ordered_lock);
> -               filemap_fdatawrite(ip->i_inode.i_mapping);
> -               spin_lock(&sdp->sd_ordered_lock);
>         }
> -       list_splice(&written, &sdp->sd_log_le_ordered);
> +       spin_unlock(&sdp->sd_ordered_lock);
> +       list_sort(NULL, &written, &ip_cmp);
> +       list_for_each_entry(ip, &written, i_ordered)
> +               ip->i_inode.i_mapping->a_ops->writepages(ip->i_inode.i_mapping,
> +                                                        &wbc);

Why not use filemap_flush followed by filemap_fdatawrite?

> +       list_for_each_entry(ip, &written, i_ordered)
> +               filemap_fdatawait(ip->i_inode.i_mapping);
> +       spin_lock(&sdp->sd_ordered_lock);
> +       list_splice_tail(&written, &sdp->sd_log_le_ordered);
>         spin_unlock(&sdp->sd_ordered_lock);
>  }

Andreas