From: Andrew Price <anprice@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [PATCH] gfs2: Don't withdraw under a spin lock
Date: Wed, 13 Jun 2018 17:32:33 +0100 [thread overview]
Message-ID: <dbefd812-a633-e475-cef7-6e6fa8035690@redhat.com> (raw)
In-Reply-To: <20180607110933.4084-1-agruenba@redhat.com>
On 07/06/18 12:09, Andreas Gruenbacher wrote:
> In two places, the gfs2_io_error_bh macro is called while holding the
> sd_ail_lock spin lock. This isn't allowed because gfs2_io_error_bh
> withdraws the filesystem, which can sleep because it issues a uevent.
> To fix that, add a __gfs2_io_error_bh macro that doesn't withdraw the
> filesystem, and withdraw the filesystem after releasing sd_ail_lock
> where necessary.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> fs/gfs2/log.c | 30 +++++++++++++++++++++---------
> fs/gfs2/util.c | 38 ++++++++++++++++++++------------------
> fs/gfs2/util.h | 10 +++++++---
> 3 files changed, 48 insertions(+), 30 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 0248835625f1..b8e937190212 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -92,7 +92,8 @@ 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)
> + struct gfs2_trans *tr,
> + bool *withdraw)
> __releases(&sdp->sd_ail_lock)
> __acquires(&sdp->sd_ail_lock)
> {
> @@ -107,8 +108,10 @@ __acquires(&sdp->sd_ail_lock)
> gfs2_assert(sdp, bd->bd_tr == tr);
>
> if (!buffer_busy(bh)) {
> - if (!buffer_uptodate(bh))
> - gfs2_io_error_bh(sdp, bh);
> + if (!buffer_uptodate(bh)) {
> + __gfs2_io_error_bh(sdp, bh);
> + *withdraw = true;
> + }
> list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
> continue;
> }
> @@ -148,6 +151,7 @@ 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);
> @@ -156,11 +160,13 @@ 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))
> + if (gfs2_ail1_start_one(sdp, wbc, tr, &withdraw))
> goto restart;
> }
> spin_unlock(&sdp->sd_ail_lock);
> blk_finish_plug(&plug);
> + if (withdraw)
> + gfs2_lm_withdraw(sdp, NULL);
> trace_gfs2_ail_flush(sdp, wbc, 0);
> }
>
> @@ -188,7 +194,8 @@ static void gfs2_ail1_start(struct gfs2_sbd *sdp)
> *
> */
>
> -static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr)
> +static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
> + bool *withdraw)
> {
> struct gfs2_bufdata *bd, *s;
> struct buffer_head *bh;
> @@ -199,11 +206,12 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr)
> gfs2_assert(sdp, bd->bd_tr == tr);
> if (buffer_busy(bh))
> continue;
> - if (!buffer_uptodate(bh))
> - gfs2_io_error_bh(sdp, bh);
> + if (!buffer_uptodate(bh)) {
> + __gfs2_io_error_bh(sdp, bh);
> + *withdraw = true;
> + }
> list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
> }
> -
> }
>
> /**
> @@ -218,10 +226,11 @@ 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);
> + gfs2_ail1_empty_one(sdp, tr, &withdraw);
> if (list_empty(&tr->tr_ail1_list) && oldest_tr)
> list_move(&tr->tr_list, &sdp->sd_ail2_list);
> else
> @@ -230,6 +239,9 @@ static int gfs2_ail1_empty(struct gfs2_sbd *sdp)
> ret = list_empty(&sdp->sd_ail1_list);
> spin_unlock(&sdp->sd_ail_lock);
>
> + if (withdraw)
> + gfs2_lm_withdraw(sdp, "fatal: I/O error(s)\n");
> +
> return ret;
> }
>
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 763d659db91b..59c811de0dc7 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -46,14 +46,16 @@ int gfs2_lm_withdraw(struct gfs2_sbd *sdp, const char *fmt, ...)
> test_and_set_bit(SDF_SHUTDOWN, &sdp->sd_flags))
> return 0;
>
> - va_start(args, fmt);
> + if (fmt) {
> + va_start(args, fmt);
>
> - vaf.fmt = fmt;
> - vaf.va = &args;
> + vaf.fmt = fmt;
> + vaf.va = &args;
>
> - fs_err(sdp, "%pV", &vaf);
> + fs_err(sdp, "%pV", &vaf);
>
> - va_end(args);
> + va_end(args);
> + }
>
> if (sdp->sd_args.ar_errors == GFS2_ERRORS_WITHDRAW) {
> fs_err(sdp, "about to withdraw this file system\n");
> @@ -246,21 +248,21 @@ int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, char *file,
> }
>
> /**
> - * gfs2_io_error_bh_i - Flag a buffer I/O error and withdraw
> - * Returns: -1 if this call withdrew the machine,
> - * 0 if it was already withdrawn
> + * gfs2_io_error_bh_i - Flag a buffer I/O error
> + * @withdraw: withdraw the filesystem
> */
>
> -int gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
> - const char *function, char *file, unsigned int line)
> +void gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
> + const char *function, char *file, unsigned int line,
> + bool withdraw)
> {
> - int rv;
> - rv = gfs2_lm_withdraw(sdp,
> - "fatal: I/O error\n"
> - " block = %llu\n"
> - " function = %s, file = %s, line = %u\n",
> - (unsigned long long)bh->b_blocknr,
> - function, file, line);
> - return rv;
> + fs_err(sdp,
> + "fatal: I/O error\n"
> + " block = %llu\n"
> + " function = %s, file = %s, line = %u\n",
> + (unsigned long long)bh->b_blocknr,
> + function, file, line);
> + if (withdraw)
> + gfs2_lm_withdraw(sdp, NULL);
> }
>
> diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h
> index 3926f95a6eb7..9d9583e842ac 100644
> --- a/fs/gfs2/util.h
> +++ b/fs/gfs2/util.h
> @@ -136,11 +136,15 @@ int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function,
> gfs2_io_error_i((sdp), __func__, __FILE__, __LINE__);
>
>
> -int gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
> - const char *function, char *file, unsigned int line);
> +void gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
> + const char *function, char *file, unsigned int line,
> + bool withdraw);
>
> #define gfs2_io_error_bh(sdp, bh) \
> -gfs2_io_error_bh_i((sdp), (bh), __func__, __FILE__, __LINE__);
> +gfs2_io_error_bh_i((sdp), (bh), __func__, __FILE__, __LINE__, true);
> +
> +#define __gfs2_io_error_bh(sdp, bh) \
> +gfs2_io_error_bh_i((sdp), (bh), __func__, __FILE__, __LINE__, false);
Perhaps these could be named something like gfs2_io_error_bh_wd and
gfs2_io_error_bh to indicate the difference.
Otherwise, this looks good to me.
Andy
>
>
> extern struct kmem_cache *gfs2_glock_cachep;
>
next prev parent reply other threads:[~2018-06-13 16:32 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-07 11:09 [Cluster-devel] [PATCH] gfs2: Don't withdraw under a spin lock Andreas Gruenbacher
2018-06-13 16:32 ` Andrew Price [this message]
2018-06-14 13:41 ` Andreas Gruenbacher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dbefd812-a633-e475-cef7-6e6fa8035690@redhat.com \
--to=anprice@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).