From: Andreas Gruenbacher <agruenba@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [PATCH] gfs2: Don't withdraw under a spin lock
Date: Thu, 14 Jun 2018 15:41:57 +0200 [thread overview]
Message-ID: <CAHc6FU4omsFPKUc9X8kgwXGkhVsQwiqDVpvAKr2DLak0R5ROMg@mail.gmail.com> (raw)
In-Reply-To: <dbefd812-a633-e475-cef7-6e6fa8035690@redhat.com>
On 13 June 2018 at 18:32, Andrew Price <anprice@redhat.com> wrote:
> 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.
Yes, makes sense. I've pushed this to the new for-later branch on
kernel.org now.
Thanks,
Andreas
prev parent reply other threads:[~2018-06-14 13:41 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
2018-06-14 13:41 ` Andreas Gruenbacher [this message]
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=CAHc6FU4omsFPKUc9X8kgwXGkhVsQwiqDVpvAKr2DLak0R5ROMg@mail.gmail.com \
--to=agruenba@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).