All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Schatzberg <schatzberg.dan@gmail.com>
To: Ming Lei <ming.lei@redhat.com>
Cc: "Jens Axboe" <axboe@kernel.dk>,
	linux-block@vger.kernel.org, "Christoph Hellwig" <hch@lst.de>,
	"Michal Koutný" <mkoutny@suse.com>
Subject: Re: [PATCH 6/6] loop: don't add worker into idle list
Date: Thu, 8 Jul 2021 10:16:50 -0400	[thread overview]
Message-ID: <YOcI0hr3k5q+/zQ4@dschatzberg-fedora-PC0Y6AEN> (raw)
In-Reply-To: <YOaiHLD74VG5I5cD@T590>

On Thu, Jul 08, 2021 at 02:58:36PM +0800, Ming Lei wrote:
> On Wed, Jul 07, 2021 at 09:55:34AM -0400, Dan Schatzberg wrote:
> > On Wed, Jul 07, 2021 at 11:19:14AM +0800, Ming Lei wrote:
> > > On Tue, Jul 06, 2021 at 09:55:36AM -0400, Dan Schatzberg wrote:
> > > > On Mon, Jul 05, 2021 at 06:26:07PM +0800, Ming Lei wrote:
> > > > >  	}
> > > > > +
> > > > > +	spin_lock(lock);
> > > > >  	list_add_tail(&cmd->list_entry, cmd_list);
> > > > > +	spin_unlock(lock);
> > > > >  	queue_work(lo->workqueue, work);
> > > > > -	spin_unlock(&lo->lo_work_lock);
> > > > >  }
> > > > >  
> > > > >  static void loop_update_rotational(struct loop_device *lo)
> > > > > @@ -1131,20 +1159,18 @@ static void loop_set_timer(struct loop_device *lo)
> > > > >  
> > > > >  static void __loop_free_idle_workers(struct loop_device *lo, bool force)
> > > > >  {
> > > > > -	struct loop_worker *pos, *worker;
> > > > > +	struct loop_worker *worker;
> > > > > +	unsigned long id;
> > > > >  
> > > > >  	spin_lock(&lo->lo_work_lock);
> > > > > -	list_for_each_entry_safe(worker, pos, &lo->idle_worker_list,
> > > > > -				idle_list) {
> > > > > +	xa_for_each(&lo->workers, id, worker) {
> > > > >  		if (!force && time_is_after_jiffies(worker->last_ran_at +
> > > > >  						LOOP_IDLE_WORKER_TIMEOUT))
> > > > >  			break;
> > > > > -		list_del(&worker->idle_list);
> > > > > -		xa_erase(&lo->workers, worker->blkcg_css->id);
> > > > > -		css_put(worker->blkcg_css);
> > > > > -		kfree(worker);
> > > > > +		if (refcount_dec_and_test(&worker->refcnt))
> > > > > +			loop_release_worker(worker);
> > > > 
> > > > This one is puzzling to me. Can't you hit this refcount decrement
> > > > superfluously each time the loop timer fires?
> > > 
> > > Not sure I get your point.
> > > 
> > > As I mentioned above, this one is the counter pair of INIT reference,
> > > but one new lo_cmd may just grab it when queueing rq before erasing the
> > > worker from xarray, so we can't release worker here until the command is
> > > completed.
> > 
> > Suppose at this point there's still an outstanding loop_cmd to be
> > serviced for this worker. The refcount_dec_and_test should decrement
> > the refcount and then fail the conditional, not calling
> > loop_release_worker. What happens if __loop_free_idle_workers fires
> > again before the loop_cmd is processed? Won't you decrement the
> > refcount again, and then end up calling loop_release_worker before the
> > loop_cmd is processed?
>  
> Good catch!
> 
> The following one line change should avoid the issue:
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 146eaa03629b..3cd51bddfec9 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -980,7 +980,6 @@ static struct loop_worker *loop_alloc_or_get_worker(struct loop_device *lo,
>  
>  static void loop_release_worker(struct loop_worker *worker)
>  {
> -	xa_erase(&worker->lo->workers, worker->blkcg_css->id);
>  	css_put(worker->blkcg_css);
>  	kfree(worker);
>  }
> @@ -1167,6 +1166,7 @@ static void __loop_free_idle_workers(struct loop_device *lo, bool force)
>  		if (!force && time_is_after_jiffies(worker->last_ran_at +
>  						LOOP_IDLE_WORKER_TIMEOUT))
>  			break;
> +		xa_erase(&worker->lo->workers, worker->blkcg_css->id);
>  		if (refcount_dec_and_test(&worker->refcnt))
>  			loop_release_worker(worker);
>  	}

Yeah, I think this resolves the issue. You could end up repeatedly
allocating workers for the same blkcg in the event that you're keeping
the worker busy for the entire LOOP_IDLE_WORKER_TIMEOUT (since it only
updates the last_ran_at when idle). You may want to add a racy check
if the refcount is > 1 to avoid that.

I think there might be a separate issue with the locking here though -
you acquire the lo->lo_work_lock in __loop_free_idle_workers and then
check worker->last_ran_at for each worker. However you only protect
the write to worker->last_ran_at (in loop_process_work) with the
worker->lock which I think means there's a potential data race on
worker->last_ran_at.

  reply	other threads:[~2021-07-08 14:16 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-05 10:26 [PATCH 0/6] loop: cleanup charging io to mem/blkcg Ming Lei
2021-07-05 10:26 ` [PATCH 1/6] loop: clean up blkcg association Ming Lei
2021-07-06  5:51   ` Christoph Hellwig
2021-07-08  7:20     ` Ming Lei
2021-07-05 10:26 ` [PATCH 2/6] loop: conver timer for monitoring idle worker into dwork Ming Lei
2021-07-06  5:52   ` Christoph Hellwig
2021-07-08  7:23     ` Ming Lei
2021-07-05 10:26 ` [PATCH 3/6] loop: add __loop_free_idle_workers() for covering freeing workers in clearing FD Ming Lei
2021-07-05 10:26 ` [PATCH 4/6] loop: improve loop_process_work Ming Lei
2021-07-06  5:54   ` Christoph Hellwig
2021-07-05 10:26 ` [PATCH 5/6] loop: use xarray to store workers Ming Lei
2021-07-05 10:26 ` [PATCH 6/6] loop: don't add worker into idle list Ming Lei
2021-07-06 13:55   ` Dan Schatzberg
2021-07-07  3:19     ` Ming Lei
2021-07-07 13:55       ` Dan Schatzberg
2021-07-08  6:58         ` Ming Lei
2021-07-08 14:16           ` Dan Schatzberg [this message]
2021-07-08 15:01             ` Ming Lei
2021-07-08 15:15               ` Dan Schatzberg
2021-07-09  0:49                 ` Ming Lei
2021-07-09 13:47                   ` Dan Schatzberg
2021-07-08 14:41           ` Dan Schatzberg

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=YOcI0hr3k5q+/zQ4@dschatzberg-fedora-PC0Y6AEN \
    --to=schatzberg.dan@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=hch@lst.de \
    --cc=linux-block@vger.kernel.org \
    --cc=ming.lei@redhat.com \
    --cc=mkoutny@suse.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 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.