Re: [PATCH v3 1/6] block: revert back to synchronous request_queue removal

From: Bart Van Assche <bvanassche@acm.org>
To: Luis Chamberlain <mcgrof@kernel.org>,
	axboe@kernel.dk, viro@zeniv.linux.org.uk,
	gregkh@linuxfoundation.org, rostedt@goodmis.org,
	mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com,
	nstange@suse.de, akpm@linux-foundation.org
Cc: mhocko@suse.com, yukuai3@huawei.com, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, Omar Sandoval <osandov@fb.com>,
	Hannes Reinecke <hare@suse.com>, Michal Hocko <mhocko@kernel.org>
Subject: Re: [PATCH v3 1/6] block: revert back to synchronous request_queue removal
Date: Fri, 1 May 2020 17:22:12 -0700	[thread overview]
Message-ID: <a2c64413-d0a4-e5c8-e0fa-904285a1189e@acm.org> (raw)
In-Reply-To: <20200429074627.5955-2-mcgrof@kernel.org>

On 2020-04-29 00:46, Luis Chamberlain wrote:
> The last reference for the request_queue must not be called from atomic
> conext. *When* the last reference to the request_queue reaches 0 varies,
  ^^^^^^
  context?
> and so let's take the opportunity to document when that is expected to
> happen and also document the context of the related calls as best as possible
> so we can avoid future issues, and with the hopes that the synchronous
> request_queue removal sticks.
> 
> We revert back to synchronous request_queue removal because asynchronous
> removal creates a regression with expected userspace interaction with
> several drivers. An example is when removing the loopback driver, one
> uses ioctls from userspace to do so, but upon return and if successful,
> one expects the device to be removed. Likewise if one races to add another
> device the new one may not be added as it is still being removed. This was
> expected behaviour before and it now fails as the device is still present
           ^^^^^^^^^
           behavior?

> +/**
> + * blk_put_queue - decrement the request_queue refcount
> + * @q: the request_queue structure to decrement the refcount for
> + *
> + * Decrements the refcount to the request_queue kobject. When this reaches 0
                              ^^
                              of?

> +/**
> + * blk_get_queue - increment the request_queue refcount
> + * @q: the request_queue structure to incremenet the refcount for
                                         ^^^^^^^^^^
                                         increment?
> + *
> + * Increment the refcount to the request_queue kobject.
                             ^^
                             of?

>  /**
> - * __blk_release_queue - release a request queue
> - * @work: pointer to the release_work member of the request queue to be released
> + * blk_release_queue - releases all allocated resources of the request_queue
> + * @kobj: pointer to a kobject, who's container is a request_queue
                                   ^^^^^
                                   whose?

> +/**
> + * disk_release - releases all allocated resources of the gendisk
> + * @dev: the device representing this disk
> + *
> + * This function releases all allocated resources of the gendisk.
> + *
> + * The struct gendisk refcounted is incremeneted with get_gendisk() or
                         ^^^^^^^^^^    ^^^^^^^^^^^^
                         refcount?     incremented?

Please fix the spelling errors. Otherwise this patch looks good to me.

Thanks,

Bart.