Date:	Mon, 25 Feb 2008 16:43:14 -0800
From:	Daniel Phillips <phillips@...nq.net>
To:	David Howells <dhowells@...hat.com>
Cc:	Trond.Myklebust@...app.com, chuck.lever@...cle.com,
	casey@...aufler-ca.com, nfsv4@...ux-nfs.org,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	selinux@...ho.nsa.gov, linux-security-module@...r.kernel.org
Subject: Re: [PATCH 00/37] Permit filesystem local caching

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists