Hello,

On Thu 18-12-08 14:15:25, Toshiyuki Okajima wrote:

quoted

quoted

quoted

From: Toshiyuki Okajima <redacted>

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
--

quoted

	/*
	 * 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 [off-list ref]
SUSE Labs, CR