Re: [PATCH] Re: zero-filed page on VOP_PUTPAGES

[yamt%mwd.biglobe.ne.jp@localhost (YAMAMOTO Takashi)]

hi,

> YAMAMOTO Takashi <yamt%mwd.biglobe.ne.jp@localhost> wrote:
> 
>> > It just prevents puffs_vnop_getattr to call uvm_vnp_setattr with a stall
>> > value between the time dosetattr sends PUFFS_VN_SETATTR and the time it
>> > calls uvm_vnp_setattr. This is enough to work around the issue if the
>> > file server does not reorder requests. 
>> but file servers can reorder requests, can't they?
> 
> It is not specified. I observed reordering in the past, but I now
> suspect that is was caused at PUFFS level, because puffs_vn_getattr()
> was working on an unlocked vnode, and therefore could slice itself
> within dosetattr() operations, and resize the file with the value it got
> from the filesystem. 
> 
> Anyway, if the fileserver reorder file size set and write operations on
> its own, I see no way it can avoid data corruption by spurious truncate.
> I guess this should be called a bug in the file server.

it makes sense.
i tend to say protocol bug, tho.

> 
>> besides that it's strongly discouraged way (i'd call it bug) to use locks,
> 
> I came up with that solution when working with the pn->pn_inresize thing
> (used to keep track of the threads involved in a pending
> PUFFS_VN_SETATTR, for whoever is joining the discussion). I noticed that
> I needed to prevent race on accesses to pn->pn_inresize. This involved
> locking the vnode in puffs_vnop_getattr(), and I realized that the lock
> itself was enough to prevent the race on file resize operations.
> 
> To sum it up: I think that puffs_vnop_getattr() must lock the vnode
> since it sends a request to the filesystem and then call
> uvm_vnp_setsize() with the size it got. Let the vnode unlocked in that
> windows is just asking for race conditions.
> 
>> the test seems broken when called with LK_SHARED held.
> 
> What happens if vn_lock(vp, LK_EXCLUSIVE|LK_RETRY) is called on a
> LK_SHARED locked vnode? I would expect it to sleep until the shared lock
> is released.

a recursive locking attempt is an error.

if we go this route, it's cleaner to use a puffs internal lock, or change
the locking protocol to require at least LK_SHARED for VOP_GETATTR.
cf. vfs_syscalls.c rev.1.350.2.3

YAMAMOTO Takashi

> 
> -- 
> Emmanuel Dreyfus
> http://hcpnet.free.fr/pubz
> manu%netbsd.org@localhost