On Monday 25 February 2008 15:19, David Howells wrote:
> So I guess there's a problem in cachefiles's efficiency - possibly due
> to the fact that it tries to be fully asynchronous.
OK, not just my imagination, and it makes me feel better about the patch
set because efficiency bugs are fixable while fundamental limitations
are not.
How much of a hurry are you in to merge this feature? You have bits
like this:
"Add a function to install a monitor on the page lock waitqueue for a
particular page, thus allowing the page being unlocked to be detected.
This is used by CacheFiles to detect read completion on a page in the
backing filesystem so that it can then copy the data to the waiting
netfs page."
We already have that hook, it is called bio_endio. My strong intuition
is that your whole mechanism should sit directly on the block device,
no matter how attractive it seems to be able to piggyback on the
namespace and layout management code of existing filesystems. I see
your current effort as the moral equivalent of FUSE: you are able to
demonstrate certain desirable behavioral properties, but you are unable
to reach full theoretical efficiency because there are layers and
layers of interface gunk interposed between the netfs user and the
cache device.
That said, I also see you have put a huge amount of work into this over
the years, it is nicely broken out, you are responsive and easy to work
with, all arguments for an early merge. Against that, you invade core
kernel for reasons that are not necessarily justified:
* two new page flags
* a new fileops method
* many changes to LSM including new object class and new hooks
* separate fs*id from task struct
* new page-private destructor hook
* probably other bits I missed
Would it be correct to say that some of these changes are to support
disconnected operation? If so, you really have two patch sets:
1) Persistent netfs cache
2) Disconnected netfs operation
You have some short snappers that look generally useful:
* add_wait_queue_tail (cool)
* write to a file without a struct file (includes ->mapping cleanup,
probably good)
* export fsync_super
Why not hunt around for existing in-kernel users that would benefit so
these can be submitted as standalone patches, shortening the remaining
patch set and partially overcoming objections due to core kernel
changes?
One thing I don't see is users coming on to lkml and saying "please
merge this, it works great for me". Since you probably have such
users, why not give them a poke?
Your cachefilesd is going to need anti-deadlock medicine like ddsnap
has. Since you don't seem at all worried about that right now, I
suspect you have not hammered this code really heavily, correct?
Without preventative measures, any memory-using daemon sitting in the
block IO path will deadlock if you hit it hard enough.
A couple of years ago you explained the purpose of the new page flags to
me and there is no way I can find that email again. Could you explain
it again please? Meanwhile I am doing my duty and reading your OLS
slides etc.
Regards,
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
