Re: [PATCH -v3] vfs: add releasepages hooks to block devices which can be used by file systems

[Jan Kara <jack@suse.cz>]

  Hello,

On Thu 18-12-08 14:15:25, Toshiyuki Okajima wrote:
> > > > From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
> > > >
> > > > Implement blkdev_releasepage() to release the buffer_heads and page
> > > > after we release private data which belongs to a client of the block
> > > > device, such as a filesystem.
> > > >
> > > > blkdev_releasepage() call the client's releasepage() which is
> > > > registered by blkdev_register_client_releasepage() to release its
> > > > private data.
> >   Yes, this is IMO the right fix. I'm just wondering about the fact that we
> > can't block in the client_releasepage(). That seems to be caused by the fact
> > that we need to be protected against client_releasepage() callback changes
> > which essentially means umount, right? I'm not saying I have a better solution
> > but introducing such limitation seems stupid just because of umount...
> >
> Difference between v2 and v3 in blkdev_releasepage:
> <		ret = (*ei->client_releasepage)(ei->client, page, wait);
> <	else
> --
> >		/*
> >		 * Since we are holding a spinlock (ei->client_lock),
> >		 * make sure the client_releasepage function
> >		 * understands that it must not block.
> >		 */
> >		ret = (*ei->client_releasepage)(ei->client, page,
> >						wait & ~__GFP_WAIT);
> >	else
>
> Ask for clarification.
  Yes, my question was more about the original design of the patch than
about the particular fix. Sorry for the confusion.

> Which of the following do you mean:
> 1) If using a spinlock in client_releasepage() is only for mount/umount,
>  this implementation is not wise.
> 2) There is the fact that a spinlock is necessary for blkdev_releasepage().
> This fact prevents us from making various implementations of
> client_releasepage().
> (Without a spinlock, we can implement a client_releasepage() which can release
> the buffers with a sleep. As a result, it may enable more buffers release than
> before.)
>
> There is the fact that a filesystem can be mounted on several places,
> and the lock mechanism is absolutely necessary for this fact.
  This is the thing I was wondering about. Why exactly is the spinlock
necessary for blkdev_releasepage()? I understand we have to protect
reading client_releasepage() pointer because it could change but my point
was that it changes only during mount / umount.

> I also think we are sad that we cannot implement various implementations for
> client_releasepage(). But now I cannot imagine what to do for
> a client_releasepage() which can sleep, too...

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