All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Andreas Gruenbacher <agruenba@redhat.com>,
	Dave Chinner <david@fromorbit.com>
Cc: "Christoph Hellwig" <hch@lst.de>,
	cluster-devel <cluster-devel@redhat.com>,
	"Mark Syms" <Mark.Syms@citrix.com>,
	"Edwin Török" <edvin.torok@citrix.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: gfs2 iomap dealock, IOMAP_F_UNBALANCED
Date: Wed, 27 Mar 2019 16:49:47 +0000	[thread overview]
Message-ID: <6e564e4b-2366-8728-c1cb-3467aad2d0f0@citrix.com> (raw)
In-Reply-To: <20190322002100.5628-1-agruenba@redhat.com>

On 3/22/19 12:21 AM, Andreas Gruenbacher wrote:
> On Fri, 22 Mar 2019 at 00:01, Andreas Gruenbacher <agruenba@redhat.com> wrote:
>> On Thu, 21 Mar 2019 at 22:43, Dave Chinner <david@fromorbit.com> wrote:
>>> The problem is calling balance_dirty_pages() inside the
>>> ->iomap_begin/->iomap_end calls and not that it is called by the
>>> iomap infrastructure itself, right?
>>>
>>> Is so, I'd prefer to see this in iomap_apply() after the call to
>>> ops->iomap_end because iomap_file_buffered_write() can iterate and
>>> call iomap_apply() multiple times. This would keep the balancing to
>>> a per-iomap granularity, rather than a per-syscall granularity.
>>>
>>> i.e. if we do write(2GB), we want more than one balancing call
>>> during that syscall, so it would be up to the filesystem to a) limit
>>> the size of write mappings to something smaller (e.g. 1024 pages)
>>> so that there are still frequent balancing calls for large writes.
>>
>> Hmm. The looping across multiple mappings isn't done in iomap_apply
>> but in iomap_file_buffered_write, so the balancing could go into
>> iomap_apply or iomap_file_buffered_write, but can't go further up the
>> stack. Given that, iomap_file_buffered_write seems the better place,
>> but this is still quite horrible.
> 
> Here's a more reasonable version of my first patch, with a cleaned up
> and hopefully fixed gfs2 part.
> 
> In addition, this checks for IOMAP_F_UNBALANCED in iomap_dirty_actor,
> the actor for iomap_file_dirty.  We don't use iomap_file_dirty in gfs2,
> but we should probably allowing to skip the dirty page balancing there
> as well.
> 
> Thanks,
> Andreas
> ---
>   fs/gfs2/bmap.c        | 64 +++++++++++++++++++++++++++++++++----------
>   fs/iomap.c            |  6 ++--
>   include/linux/iomap.h |  1 +
>   3 files changed, 55 insertions(+), 16 deletions(-)
> 
Thanks, this fixes the reported deadlock. I haven't yet checked whether 
it has any performance impact.

Tested-by: Ross Lagerwall <ross.lagerwall@citrix.com>

WARNING: multiple messages have this Message-ID (diff)
From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] gfs2 iomap dealock, IOMAP_F_UNBALANCED
Date: Wed, 27 Mar 2019 16:49:47 +0000	[thread overview]
Message-ID: <6e564e4b-2366-8728-c1cb-3467aad2d0f0@citrix.com> (raw)
In-Reply-To: <20190322002100.5628-1-agruenba@redhat.com>

On 3/22/19 12:21 AM, Andreas Gruenbacher wrote:
> On Fri, 22 Mar 2019 at 00:01, Andreas Gruenbacher <agruenba@redhat.com> wrote:
>> On Thu, 21 Mar 2019 at 22:43, Dave Chinner <david@fromorbit.com> wrote:
>>> The problem is calling balance_dirty_pages() inside the
>>> ->iomap_begin/->iomap_end calls and not that it is called by the
>>> iomap infrastructure itself, right?
>>>
>>> Is so, I'd prefer to see this in iomap_apply() after the call to
>>> ops->iomap_end because iomap_file_buffered_write() can iterate and
>>> call iomap_apply() multiple times. This would keep the balancing to
>>> a per-iomap granularity, rather than a per-syscall granularity.
>>>
>>> i.e. if we do write(2GB), we want more than one balancing call
>>> during that syscall, so it would be up to the filesystem to a) limit
>>> the size of write mappings to something smaller (e.g. 1024 pages)
>>> so that there are still frequent balancing calls for large writes.
>>
>> Hmm. The looping across multiple mappings isn't done in iomap_apply
>> but in iomap_file_buffered_write, so the balancing could go into
>> iomap_apply or iomap_file_buffered_write, but can't go further up the
>> stack. Given that, iomap_file_buffered_write seems the better place,
>> but this is still quite horrible.
> 
> Here's a more reasonable version of my first patch, with a cleaned up
> and hopefully fixed gfs2 part.
> 
> In addition, this checks for IOMAP_F_UNBALANCED in iomap_dirty_actor,
> the actor for iomap_file_dirty.  We don't use iomap_file_dirty in gfs2,
> but we should probably allowing to skip the dirty page balancing there
> as well.
> 
> Thanks,
> Andreas
> ---
>   fs/gfs2/bmap.c        | 64 +++++++++++++++++++++++++++++++++----------
>   fs/iomap.c            |  6 ++--
>   include/linux/iomap.h |  1 +
>   3 files changed, 55 insertions(+), 16 deletions(-)
> 
Thanks, this fixes the reported deadlock. I haven't yet checked whether 
it has any performance impact.

Tested-by: Ross Lagerwall <ross.lagerwall@citrix.com>



  reply	other threads:[~2019-03-27 16:49 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-21 13:13 gfs2 iomap dealock, IOMAP_F_UNBALANCED Andreas Gruenbacher
2019-03-21 13:13 ` [Cluster-devel] " Andreas Gruenbacher
2019-03-21 21:43 ` Dave Chinner
2019-03-21 21:43   ` [Cluster-devel] " Dave Chinner
2019-03-21 23:01   ` Andreas Gruenbacher
2019-03-21 23:01     ` [Cluster-devel] " Andreas Gruenbacher
2019-03-22  0:21   ` Andreas Gruenbacher
2019-03-22  0:21     ` [Cluster-devel] " Andreas Gruenbacher
2019-03-27 16:49     ` Ross Lagerwall [this message]
2019-03-27 16:49       ` Ross Lagerwall
2019-03-28 16:51 ` Christoph Hellwig
2019-03-28 16:51   ` [Cluster-devel] " Christoph Hellwig
2019-03-29 22:13   ` Andreas Gruenbacher
2019-03-29 22:13     ` [Cluster-devel] " Andreas Gruenbacher
2019-04-07  7:32     ` Christoph Hellwig
2019-04-07  7:32       ` [Cluster-devel] " Christoph Hellwig
2019-04-08  8:53       ` Andreas Gruenbacher
2019-04-08  8:53         ` [Cluster-devel] " Andreas Gruenbacher
2019-04-08 13:44         ` Jan Kara
2019-04-08 13:44           ` [Cluster-devel] " Jan Kara
2019-04-09 12:15           ` Christoph Hellwig
2019-04-09 12:15             ` [Cluster-devel] " Christoph Hellwig
2019-04-09 12:27             ` Andreas Gruenbacher
2019-04-09 12:27               ` [Cluster-devel] " 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=6e564e4b-2366-8728-c1cb-3467aad2d0f0@citrix.com \
    --to=ross.lagerwall@citrix.com \
    --cc=Mark.Syms@citrix.com \
    --cc=agruenba@redhat.com \
    --cc=cluster-devel@redhat.com \
    --cc=david@fromorbit.com \
    --cc=edvin.torok@citrix.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.