Subject: Re: Redoing file system suspension API (update)
To: None <hannken@eis.cs.tu-bs.de>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 06/18/2006 22:59:33
thanks for taking a look of this.
although i have many negative comments, i think it's a step forward. :-)


first of all, i tend to think filesystem snapshot thing should be done
entirely in filesystem-dependent code.

> - Instead of tracking possible long sleeps I put the suspend/resume into
>   ltsleep().

why?

i don't think it's desirable for each subsystems to put their own
random hooks in these places.

what happens if a filesystem itself sleeps with PCATCH?
(maybe you can call it a bug, but we currently have such a code.)

besides, vngate_resume itself seems to sleep.
is it safe to call it from ltsleep?

> To solve the rest of 3) it adds a throttling on the first gate not involved
> in a suspending file system.

- isn't it normal that an operation become slow when the system has
  other activities?
- why you check P_SYSTEM?

> ** The new API is:
> 
> Using explicit enter()/leave() pairs adds much complexity so I took another
> approach. I use two types of gates.  Normal gates need a "leave" operation.
> Permanent gates are valid until the thread returns to user mode.

while it can make your patch smaller, i think it's actually more complex
and harder to understand and maintain.

> void vngate_initialize(struct lwp *l)
> 
>   Initialize the per-thread state.  Will be freed on vngate_leave_all(..., 1).

please try to avoid putting subsystem-specific data to struct lwp.

>   V_NOERROR	Panic on error.  No need for the caller to check the result.

what's the point of this?

> Adding permanent gates to lookup(), (new) FILE_USE_GATED() and nfsrv_fhtovp()
> covers most system calls. The various compat parts are not ready.  A scan over
> all "FILE_USE()" should be sufficient here.  The implementation renames
> "vfs_write_XXX" to "vfs_XXX" and adds the state pointer to "struct lwp".
> All other changes depend on "option NEWVNGATE" to make it easy to test the
> new API.

why you put vngate_enter into FILE_USE, rather than VOPs?

YAMAMOTO Takashi