linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Josef Bacik <josef@toxicpanda.com>
To: fdmanana@kernel.org
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] Btrfs: remove unnecessary delalloc mutex for inodes
Date: Fri, 25 Oct 2019 08:08:44 -0400	[thread overview]
Message-ID: <20191025120843.ujydwo3w3twmdl3o@MacBook-Pro-91.local> (raw)
In-Reply-To: <20191025095242.15996-1-fdmanana@kernel.org>

On Fri, Oct 25, 2019 at 10:52:42AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The inode delalloc mutex was added a long time ago by commit f248679e86fea
> ("Btrfs: add a delalloc mutex to inodes for delalloc reservations"), and
> the reason for its introduction is not very clear from the change log. It
> claims it solves bogus warnings from lockdep, however it lacks an example
> report/warning from lockdep, or any explanation.
> 
> Since we have enough concurrentcy protection from the locks of the space
> info and block reserve objects, and such lockdep warnings don't seem to
> exist anymore (at least on a 5.3 kernel I couldn't get them with fstests,
> ltp, fs_mark, etc), remove it, simplifying things a bit and decreasing
> the size of the btrfs_inode structure. With some quick fio tests doing
> direct IO and mmap writes I couldn't observe any significant performance
> increase either (direct IO writes that don't increase the file's size
> don't hold the inode's lock for their entire duration and mmap writes
> don't hold the inode's lock at all), which are the only type of writes
> that could see any performance gain due to less serialization.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

The problem was taking the i_mutex in mmap, which is how I was protecting
delalloc reservations originally.  The delalloc mutex didn't come with all of
the other dependencies.  That's what the lockdep messages were about, removing
the lock isn't going to make them appear again.

We _had_ to lock around this because we used to do tricks to keep from
over-reserving, and if we didn't serialize delalloc reservations we'd end up
with ugly accounting problems when we tried to clean things up.

However with my recentish changes this isn't the case anymore.  Every operation
is responsible for reserving its space, and then adding it to the inode.  Then
cleaning up is straightforward and can't be mucked up by other users.  So we no
longer need the delalloc mutex to safe us from ourselves.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

  reply	other threads:[~2019-10-25 12:08 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-25  9:52 [PATCH] Btrfs: remove unnecessary delalloc mutex for inodes fdmanana
2019-10-25 12:08 ` Josef Bacik [this message]
2019-10-25 12:11   ` Filipe Manana
2019-10-25 16:32 ` David Sterba

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=20191025120843.ujydwo3w3twmdl3o@MacBook-Pro-91.local \
    --to=josef@toxicpanda.com \
    --cc=fdmanana@kernel.org \
    --cc=linux-btrfs@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 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).