Re: [PATCH] Btrfs: remove unnecessary delalloc mutex for inodes

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Oct 25, 2019 at 1:08 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> 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.

Yes, thanks. That's what I thought, and couldn't see any reason for it
being needed given the current (much better) way of reserving space.

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