All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shunki Fujita <shunki-fujita@cybozu.co.jp>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	viro@zeniv.linux.org.uk, Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH] fs: don't flush pagecache when expanding block device
Date: Mon, 19 Mar 2018 19:52:10 +0900	[thread overview]
Message-ID: <0_14961900_1521456730.19987.cbgrn@bozuman.cybozu.com> (raw)
In-Reply-To: <20180316152256.2f4883edc3cb6b374bcdd448@linux-foundation.org>

Hi Andrew,

> On Fri, 16 Mar 2018 15:55:53 +0900 shunki-fujita <shunki-fujita@cybozu.co.jp> wrote:
> 
> > When changing the size of a block device, its all caches are freed.
> > It's necessary on shrinking to prevent spurious I/Os to the disappeared region.
> > However, on expanding, such kind of I/Os doesn't happen.
> > 
> > Similar things can be considered for btrfs filesystem resize and resize2fs,
> > but they are designed not to cache drops when expanding.
> > Therefore this patch removes unnecessary cache drop.
> 
> Yes, the patch removes the flush_disk() call when shrinking.  But it
> adds additional code which is unchangelogged and uncommented.  What's
> happening here?
> 
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1337,7 +1337,14 @@ void check_disk_size_change(struct gendisk *disk, struct block_device *bdev)
> >  		       "%s: detected capacity change from %lld to %lld\n",
> >  		       disk->disk_name, bdev_size, disk_size);
> >  		i_size_write(bdev->bd_inode, disk_size);
> > -		flush_disk(bdev, false);
> > +		if (bdev_size > disk_size) {
> > +			flush_disk(bdev, false);
> > +		} else {
> > +			if (!bdev->bd_disk)
> > +				return;
> > +			if (disk_part_scan_enabled(bdev->bd_disk))
> > +				bdev->bd_invalidated = 1;
> > +		}
> >  	}
> >  }
> 
> Oh, I see.  It's a copy-n-paste of some mystery uncommented,
> lost-in-the-mists-of-time code from flush_disk().  Argh.
> 
> The world would be a better place if we could move that bit of code
> into a separate function, figure out what it does, document it and call
> it from both sites.
> 

This was an unnecessary additional code.
Since bdev-> bd_invalidated always 0 after check_disk_size_change (), no additional code was required.
https://github.com/torvalds/linux/blob/master/fs/block_dev.c#L1365-L1366

I will repost the fix patch.

Thanks,
Shunki

  parent reply	other threads:[~2018-03-19 10:52 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-16  6:55 [PATCH] fs: don't flush pagecache when expanding block device shunki-fujita
2018-03-16 22:22 ` Andrew Morton
2018-03-19 10:52 ` Shunki Fujita [this message]
2018-03-19 11:00 ` [PATCH v2] " shunki-fujita

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=0_14961900_1521456730.19987.cbgrn@bozuman.cybozu.com \
    --to=shunki-fujita@cybozu.co.jp \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /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.