Re: [PATCH 3/8] block: remove the racy bd_inode->i_mapping->nrpages asserts

[Jan Kara <jack@suse.cz>]

On Fri 28-01-22 08:26:14, Christoph Hellwig wrote:
> On Thu, Jan 27, 2022 at 01:23:27PM +0100, Jan Kara wrote:
> > On Thu 27-01-22 10:49:42, Christoph Hellwig wrote:
> > > On Thu, Jan 27, 2022 at 10:47:37AM +0100, Jan Kara wrote:
> > > > On Wed 26-01-22 16:50:35, Christoph Hellwig wrote:
> > > > > Nothing prevents a file system or userspace opener of the block device
> > > > > from redirtying the page right afte sync_blockdev returned.  Fortunately
> > > > > data in the page cache during a block device change is mostly harmless
> > > > > anyway.
> > > > > 
> > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > 
> > > > My understanding was these warnings are there to tell userspace it is doing
> > > > something wrong. Something like the warning we issue when DIO races with
> > > > buffered IO... I'm not sure how useful they are but I don't see strong
> > > > reason to remove them either...
> > > 
> > > Well, it is not just a warning, but also fails the command.  With some of
> > > the reduced synchronization blktests loop/002 can hit them pretty reliably.
> > 
> > I see. I guess another place where using mapping->invalidate_lock would be
> > good to avoid these races... So maybe something like attached patch?
> 
> So this looks sensible, but it does nest the inode lock and and the
> invalidate lockinside lo_mutex.  I wonder if that is going to create
> more problems down the road.

Yeah, I was wondering about that a bit as well. Quick blktests run didn't
trigger any lockdep warning so I thought it's worth a try. But for now I
guess let's not complicate things, it's difficult enough already.

I have checked the code now and it does not seem to cause any obvious harm
if we have block device page cache while changing loop device parameters.
Just some IO may fail, trigger some warning (e.g. IO beyond end of device)
but nothing worse.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR