On Tue, Dec 04, 2018 at 01:46:58PM +0200, Nikolay Borisov wrote:
>
>
> On 3.12.18 г. 18:06 ч., Josef Bacik wrote:
> > The throttle path doesn't take cleaner_delayed_iput_mutex, which means
> > we could think we're done flushing iputs in the data space reservation
> > path when we could have a throttler doing an iput. There's no real
> > reason to serialize the delayed iput flushing, so instead of taking the
> > cleaner_delayed_iput_mutex whenever we flush the delayed iputs just
> > replace it with an atomic counter and a waitqueue. This removes the
> > short (or long depending on how big the inode is) window where we think
> > there are no more pending iputs when there really are some.
> >
> > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > ---
> > fs/btrfs/ctree.h | 4 +++-
> > fs/btrfs/disk-io.c | 5 ++---
> > fs/btrfs/extent-tree.c | 13 ++++++++-----
> > fs/btrfs/inode.c | 21 +++++++++++++++++++++
> > 4 files changed, 34 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index dc56a4d940c3..20af5d6d81f1 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -915,7 +915,8 @@ struct btrfs_fs_info {
> >
> > spinlock_t delayed_iput_lock;
> > struct list_head delayed_iputs;
> > - struct mutex cleaner_delayed_iput_mutex;
> > + atomic_t nr_delayed_iputs;
> > + wait_queue_head_t delayed_iputs_wait;
> >
>
> Have you considered whether the same could be achieved with a completion
> rather than an open-coded waitqueue? I tried prototyping it and it could
> be done but it becomes messy regarding when the completion should be
> initialised i.e only when it's not in btrfs_add_delayed_iput
>
Yeah a waitqueue makes more sense here than a completion since it's not a
one-off.
>
> <snip>
>
> > @@ -4958,9 +4962,8 @@ static void flush_space(struct btrfs_fs_info *fs_info,
> > * bunch of pinned space, so make sure we run the iputs before
> > * we do our pinned bytes check below.
> > */
> > - mutex_lock(&fs_info->cleaner_delayed_iput_mutex);
> > btrfs_run_delayed_iputs(fs_info);
> > - mutex_unlock(&fs_info->cleaner_delayed_iput_mutex);
> > + btrfs_wait_on_delayed_iputs(fs_info);
>
> Waiting on delayed iputs here is pointless since they are run
> synchronously form this context.
>
Unless there are other threads (the cleaner thread) running iputs as well. We
could be running an iput that is evicting the inode in another thread and we
really want that space, so we need to wait here to make sure everybody is truly
done. Thanks,
Josef
