On Sun, Jan 16, 2005 at 12:32:33PM +0100, Matthias Drochner wrote:
> 
> Thanks for fixing this -- I can't test right now, will do
> that next week.
> 
> chuq@chuq.com said:
> > I would say that any copyin() that reads from the kernel side that
> > doesn't have to is broken, so it's only an issue on alpha. 
> 
> Well, I wouldn't call it "broken", it is just unusual.
> The VM system just shouldn't make assumptions about
> implementation details of md code.

with the change I checked in, that particular assumption is no longer made.

the alpha implementation is unavoidable because of the hardware limitations,
but the versions for other hardware without that limitation should not
needlessly read while writing.


> For me, it still doesn't look as clean as it should. The
> use of "access_type" overloads the VM level permission
> control with higher level filesystem semantics.
> How about using a bit of "flags" in ubc_fault() and the
> pager/fs functions called to tell "this is for a filesystem
> write", and let access_type still tell the true type
> of the trap? After all, the access_type is named "VM_PROT_*",
> so it should be just this instead of a filesystem access type.
> (At that point I'm asking myself whether the VM access_type
> should be passed to the filesystem vop_getpages at all. Cleaner
> would be something with filesystem semantics. Just an idea,
> didn't look deeper.)

think of access_type as the logical type of VM access, not the particular
type of trap that was taken.  these are usually the same, but now we've
found a case where they're not.  as you say, the pager_get / VOP_GETPAGES
code has no interest in the actual trap type.

the use of VM_PROT_* to refer to types of VM access as well as permissions
is ubiquitous in our code, and it seems sensible enough.


> > I checked in a different fix for this issue,
> > update uvm_bio.c.
> 
> This is more or less what I meant with
> > A flag set somewhere in the upper
> > layer, where we still know that we are doing a write(),
> 
> I was concerned about concurrency, but since the whole thing
> is locked anyway at vnode level this is probably not an issue.
> (And there was already state kept between the ubc_alloc()
> and the fault handler.)

right, the existing locking protects this as well.

-Chuck