Re: [Cluster-devel] [GFS2 PATCH 1/9] gfs2: Introduce concept of a pending withdraw

Steven Whitehouse Fri, 15 Feb 2019 03:38:12 -0800


On 13/02/2019 15:21, Bob Peterson wrote:

File system withdraws can be delayed when inconsistencies are
discovered when we cannot withdraw immediately, for example, when
critical spin_locks are held. But delaying the withdraw can cause
gfs2 to ignore the error and keep running for a short period of time.
For example, an rgrp glock may be dequeued and demoted while there
are still buffers that haven't been properly revoked, due to io
errors writing to the journal.

This patch introduces a new concept of a delayed withdraw, which
means an inconsistency has been discovered and we need to withdraw
at the earliest possible opportunity. In these cases, we aren't
quite withdrawn yet, but we still need to not dequeue glocks and
other critical things. If we dequeue the glocks and the withdraw
results in our journal being replayed, the replay could overwrite
data that's been modified by a different node that acquired the
glock in the meantime.

Signed-off-by: Bob Peterson <rpete...@redhat.com>

This withdrawn() wrapper seems like a good plan anyway, since it helpsmake the code more readable,


Steve.

---
  fs/gfs2/aops.c       |  4 ++--
  fs/gfs2/file.c       |  2 +-
  fs/gfs2/glock.c      |  7 +++----
  fs/gfs2/glops.c      |  2 +-
  fs/gfs2/incore.h     |  1 +
  fs/gfs2/log.c        | 20 ++++++++------------
  fs/gfs2/meta_io.c    |  6 +++---
  fs/gfs2/ops_fstype.c |  3 +--
  fs/gfs2/quota.c      |  2 +-
  fs/gfs2/super.c      |  6 +++---
  fs/gfs2/sys.c        |  2 +-
  fs/gfs2/util.c       |  1 +
  fs/gfs2/util.h       |  8 ++++++++
  13 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 05dd78f4b2b3..0d3cde8a61cd 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -521,7 +521,7 @@ static int __gfs2_readpage(void *file, struct page *page)
                error = mpage_readpage(page, gfs2_block_map);
        }

- if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))

+       if (unlikely(withdrawn(sdp)))
                return -EIO;

return error;

@@ -638,7 +638,7 @@ static int gfs2_readpages(struct file *file, struct 
address_space *mapping,
        gfs2_glock_dq(&gh);
  out_uninit:
        gfs2_holder_uninit(&gh);
-       if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
+       if (unlikely(withdrawn(sdp)))
                ret = -EIO;
        return ret;
  }
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index a2dea5bc0427..4046f6ac7f13 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -1169,7 +1169,7 @@ static int gfs2_lock(struct file *file, int cmd, struct 
file_lock *fl)
                cmd = F_SETLK;
                fl->fl_type = F_UNLCK;
        }
-       if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags))) {
+       if (unlikely(withdrawn(sdp))) {
                if (fl->fl_type == F_UNLCK)
                        locks_lock_file_wait(file, fl);
                return -EIO;
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f66773c71bcd..c6d6e478f5e3 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -542,7 +542,7 @@ __acquires(&gl->gl_lockref.lock)
        unsigned int lck_flags = (unsigned int)(gh ? gh->gh_flags : 0);
        int ret;

- if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) &&

+       if (unlikely(withdrawn(sdp)) &&
            target != LM_ST_UNLOCKED)
                return;
        lck_flags &= (LM_FLAG_TRY | LM_FLAG_TRY_1CB | LM_FLAG_NOEXP |
@@ -579,8 +579,7 @@ __acquires(&gl->gl_lockref.lock)
                }
                else if (ret) {
                        fs_err(sdp, "lm_lock ret %d\n", ret);
-                       GLOCK_BUG_ON(gl, !test_bit(SDF_SHUTDOWN,
-                                                  &sdp->sd_flags));
+                       GLOCK_BUG_ON(gl, !withdrawn(sdp));
                }
        } else { /* lock_nolock */
                finish_xmote(gl, target);
@@ -1092,7 +1091,7 @@ int gfs2_glock_nq(struct gfs2_holder *gh)
        struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
        int error = 0;

- if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))

+       if (unlikely(withdrawn(sdp)))
                return -EIO;

if (test_bit(GLF_LRU, &gl->gl_flags))

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index f15b4c57c4bd..9c86c8004ba7 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -539,7 +539,7 @@ static int freeze_go_xmote_bh(struct gfs2_glock *gl, struct 
gfs2_holder *gh)
                        gfs2_consist(sdp);

/* Initialize some head of the log stuff */

-               if (!test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) {
+               if (!withdrawn(sdp)) {
                        sdp->sd_log_sequence = head.lh_sequence + 1;
                        gfs2_log_pointers_init(sdp, head.lh_blkno);
                }
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index cdf07b408f54..8380d4db8be6 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -621,6 +621,7 @@ enum {
        SDF_SKIP_DLM_UNLOCK     = 8,
        SDF_FORCE_AIL_FLUSH     = 9,
        SDF_AIL1_IO_ERROR       = 10,
+       SDF_PENDING_WITHDRAW    = 11, /* Will withdraw eventually */
  };

enum gfs2_freeze_state {

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 5bfaf381921a..ec8675113b0d 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -92,8 +92,7 @@ static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)

static int gfs2_ail1_start_one(struct gfs2_sbd *sdp,

                               struct writeback_control *wbc,
-                              struct gfs2_trans *tr,
-                              bool *withdraw)
+                              struct gfs2_trans *tr)
  __releases(&sdp->sd_ail_lock)
  __acquires(&sdp->sd_ail_lock)
  {
@@ -112,7 +111,7 @@ __acquires(&sdp->sd_ail_lock)
                            !test_and_set_bit(SDF_AIL1_IO_ERROR,
                                              &sdp->sd_flags)) {
                                gfs2_io_error_bh(sdp, bh);
-                               *withdraw = true;
+                               set_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags);
                        }
                        list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
                        continue;
@@ -153,7 +152,6 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct 
writeback_control *wbc)
        struct list_head *head = &sdp->sd_ail1_list;
        struct gfs2_trans *tr;
        struct blk_plug plug;
-       bool withdraw = false;

trace_gfs2_ail_flush(sdp, wbc, 1);

        blk_start_plug(&plug);
@@ -162,12 +160,12 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct 
writeback_control *wbc)
        list_for_each_entry_reverse(tr, head, tr_list) {
                if (wbc->nr_to_write <= 0)
                        break;
-               if (gfs2_ail1_start_one(sdp, wbc, tr, &withdraw))
+               if (gfs2_ail1_start_one(sdp, wbc, tr))
                        goto restart;
        }
        spin_unlock(&sdp->sd_ail_lock);
        blk_finish_plug(&plug);
-       if (withdraw)
+       if (test_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags))
                gfs2_lm_withdraw(sdp, NULL);
        trace_gfs2_ail_flush(sdp, wbc, 0);
  }
@@ -196,8 +194,7 @@ static void gfs2_ail1_start(struct gfs2_sbd *sdp)
   *
   */

-static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,

-                               bool *withdraw)
+static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr)
  {
        struct gfs2_bufdata *bd, *s;
        struct buffer_head *bh;
@@ -211,7 +208,7 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, 
struct gfs2_trans *tr,
                if (!buffer_uptodate(bh) &&
                    !test_and_set_bit(SDF_AIL1_IO_ERROR, &sdp->sd_flags)) {
                        gfs2_io_error_bh(sdp, bh);
-                       *withdraw = true;
+                       set_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags);
                }
                list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
        }
@@ -229,11 +226,10 @@ static int gfs2_ail1_empty(struct gfs2_sbd *sdp)
        struct gfs2_trans *tr, *s;
        int oldest_tr = 1;
        int ret;
-       bool withdraw = false;

spin_lock(&sdp->sd_ail_lock);

        list_for_each_entry_safe_reverse(tr, s, &sdp->sd_ail1_list, tr_list) {
-               gfs2_ail1_empty_one(sdp, tr, &withdraw);
+               gfs2_ail1_empty_one(sdp, tr);
                if (list_empty(&tr->tr_ail1_list) && oldest_tr)
                        list_move(&tr->tr_list, &sdp->sd_ail2_list);
                else
@@ -242,7 +238,7 @@ static int gfs2_ail1_empty(struct gfs2_sbd *sdp)
        ret = list_empty(&sdp->sd_ail1_list);
        spin_unlock(&sdp->sd_ail_lock);

- if (withdraw)

+       if (test_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags))
                gfs2_lm_withdraw(sdp, "fatal: I/O error(s)\n");

return ret;

diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index be9c0bf697fe..97c161782763 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -254,7 +254,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int 
flags,
        struct buffer_head *bh, *bhs[2];
        int num = 0;

- if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags))) {

+       if (unlikely(withdrawn(sdp))) {
                *bhp = NULL;
                return -EIO;
        }
@@ -312,7 +312,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int 
flags,

int gfs2_meta_wait(struct gfs2_sbd *sdp, struct buffer_head *bh)

  {
-       if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
+       if (unlikely(withdrawn(sdp)))
                return -EIO;

wait_on_buffer(bh);

@@ -323,7 +323,7 @@ int gfs2_meta_wait(struct gfs2_sbd *sdp, struct buffer_head 
*bh)
                        gfs2_io_error_bh_wd(sdp, bh);
                return -EIO;
        }
-       if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
+       if (unlikely(withdrawn(sdp)))
                return -EIO;

return 0;

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 1179763f6370..402201978312 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -999,8 +999,7 @@ static int gfs2_lm_mount(struct gfs2_sbd *sdp, int silent)
  void gfs2_lm_unmount(struct gfs2_sbd *sdp)
  {
        const struct lm_lockops *lm = sdp->sd_lockstruct.ls_ops;
-       if (likely(!test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) &&
-           lm->lm_unmount)
+       if (likely(!withdrawn(sdp)) && lm->lm_unmount)
                lm->lm_unmount(sdp);
  }

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c

index 2ae5a109eea7..5b48489b0469 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1478,7 +1478,7 @@ static void quotad_error(struct gfs2_sbd *sdp, const char 
*msg, int error)
  {
        if (error == 0 || error == -EROFS)
                return;
-       if (!test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) {
+       if (!withdrawn(sdp)) {
                fs_err(sdp, "gfs2_quotad: %s error %d\n", msg, error);
                sdp->sd_log_error = error;
                wake_up(&sdp->sd_logd_waitq);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index d4b11c903971..8033f24e0ad0 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -803,7 +803,7 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)

if (!(flags & I_DIRTY_INODE))

                return;
-       if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
+       if (unlikely(withdrawn(sdp)))
                return;
        if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {
                ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
@@ -852,7 +852,7 @@ static int gfs2_make_fs_ro(struct gfs2_sbd *sdp)

error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED, GL_NOCACHE,

                                   &freeze_gh);
-       if (error && !test_bit(SDF_SHUTDOWN, &sdp->sd_flags))
+       if (error && !withdrawn(sdp))
                return error;

flush_workqueue(gfs2_delete_workqueue);

@@ -1006,7 +1006,7 @@ static int gfs2_freeze(struct super_block *sb)
        if (atomic_read(&sdp->sd_freeze_state) != SFS_UNFROZEN)
                goto out;

- if (test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) {

+       if (withdrawn(sdp)) {
                error = -EINVAL;
                goto out;
        }
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 1787d295834e..a2419e92a64e 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -121,7 +121,7 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const 
char *buf, size_t len)

static ssize_t withdraw_show(struct gfs2_sbd *sdp, char *buf)

  {
-       unsigned int b = test_bit(SDF_SHUTDOWN, &sdp->sd_flags);
+       unsigned int b = withdrawn(sdp);
        return snprintf(buf, PAGE_SIZE, "%u\n", b);
  }

diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c

index 0a814ccac41d..717aef772c60 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -47,6 +47,7 @@ int gfs2_lm_withdraw(struct gfs2_sbd *sdp, const char *fmt, 
...)
            test_and_set_bit(SDF_SHUTDOWN, &sdp->sd_flags))
                return 0;

+ clear_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags);

        if (fmt) {
                va_start(args, fmt);

diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h

index 9278fecba632..16e087da3bd3 100644
--- a/fs/gfs2/util.h
+++ b/fs/gfs2/util.h
@@ -167,6 +167,14 @@ static inline unsigned int gfs2_tune_get_i(struct 
gfs2_tune *gt,
        return x;
  }

+static inline bool withdrawn(struct gfs2_sbd *sdp)

+{
+       if (test_bit(SDF_SHUTDOWN, &sdp->sd_flags) ||
+           test_bit(SDF_PENDING_WITHDRAW, &sdp->sd_flags))
+               return true;
+       return false;
+}
+
  #define gfs2_tune_get(sdp, field) \
  gfs2_tune_get_i(&(sdp)->sd_tune, &(sdp)->sd_tune.field)

Re: [Cluster-devel] [GFS2 PATCH 1/9] gfs2: Introduce concept of a pending withdraw

Reply via email to