[PATCH] loop: Only change blocksize when needed.

[PATCH] loop: Only change blocksize when needed.

[Martijn Coenen]

Return early in loop_set_block_size() if the requested block size is
identical to the one we already have; this avoids expensive calls to
freeze the block queue.

Signed-off-by: Martijn Coenen <maco@android.com>
---
 drivers/block/loop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c1c844ad6b1a..a42c49e04954 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1541,16 +1541,16 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
 		return -EINVAL;
 
-	if (lo->lo_queue->limits.logical_block_size != arg) {
-		sync_blockdev(lo->lo_device);
-		kill_bdev(lo->lo_device);
-	}
+	if (lo->lo_queue->limits.logical_block_size == arg)
+		return 0;
+
+	sync_blockdev(lo->lo_device);
+	kill_bdev(lo->lo_device);
 
 	blk_mq_freeze_queue(lo->lo_queue);
 
 	/* kill_bdev should have truncated all the pages */
-	if (lo->lo_queue->limits.logical_block_size != arg &&
-			lo->lo_device->bd_inode->i_mapping->nrpages) {
+	if (lo->lo_device->bd_inode->i_mapping->nrpages) {
 		err = -EAGAIN;
 		pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
 			__func__, lo->lo_number, lo->lo_file_name,
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH] loop: Only change blocksize when needed.

[Christoph Hellwig]

On Tue, Mar 10, 2020 at 02:12:30PM +0100, Martijn Coenen wrote:
> Return early in loop_set_block_size() if the requested block size is
> identical to the one we already have; this avoids expensive calls to
> freeze the block queue.
> 
> Signed-off-by: Martijn Coenen <maco@android.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] loop: Only change blocksize when needed.

[Jens Axboe]

On 3/10/20 7:12 AM, Martijn Coenen wrote:
> Return early in loop_set_block_size() if the requested block size is
> identical to the one we already have; this avoids expensive calls to
> freeze the block queue.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] loop: Only change blocksize when needed.

[Chaitanya Kulkarni]

Logically this is a right thing to do, but I wonder how much speedup
you are getting with these improvements ?
It will be great if you have some numbers so we all know the speedup.

Irrespective of that, this looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 03/10/2020 06:17 AM, Martijn Coenen wrote:
> Return early in loop_set_block_size() if the requested block size is
> identical to the one we already have; this avoids expensive calls to
> freeze the block queue.
>
> Signed-off-by: Martijn Coenen<maco@android.com>

Re: [PATCH] loop: Only change blocksize when needed.

[Martijn Coenen]

Hi Chaitanya,

On Tue, Mar 10, 2020 at 9:23 PM Chaitanya Kulkarni
<Chaitanya.Kulkarni@wdc.com> wrote:
>
> Logically this is a right thing to do, but I wonder how much speedup
> you are getting with these improvements ?
> It will be great if you have some numbers so we all know the speedup.

What makes blk_mq_freeze_queue() relatively expensive is that the
implementation of that function calls synchronize_rcu() (for good
reasons); on our x86 devices, I've seen that take 15-20ms on average.
Recent Android versions configure a lot (~30) of loop devices at boot,
and so this saves us about 600ms of boot time. I strongly suspect this
benefits other usecases besides Android.

There is another call in loop_set_status() which is harder to remove;
eg, if you use loop_set_status() to change the offset of a loop
device, you shouldn't do that if there are still requests outstanding,
and blk_mq_freeze_queue() ensures that. But in our specific case, we
know that there won't be requests outstanding, because during this
phase of boot the loop device hasn't been mounted yet. But we can't
tell the kernel that. So I will follow up with a patch that tries to
address that issue, which will also have some more detailed numbers.
If you have ideas or suggestions, feel free to let me know!

Thanks,
Martijn

>
> Irrespective of that, this looks good to me.
>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>
> On 03/10/2020 06:17 AM, Martijn Coenen wrote:
> > Return early in loop_set_block_size() if the requested block size is
> > identical to the one we already have; this avoids expensive calls to
> > freeze the block queue.
> >
> > Signed-off-by: Martijn Coenen<maco@android.com>
>