[9fans] bind vs. 9660srv

[9fans] bind vs. 9660srv

[erik quanstrom]

i mentioned a few days ago that bind and 9660srv were not
agreeing.  i tracked this down a bit further and am now
quite confused.  here's the initial symptom

	; 9660srv
	; mount /srv/9660 /n/cd0 /n/other/ftp/plan9.iso
	; du -s /n/cd0/386/bin
	80143	/n/cd1/386/bin

but then through if i bind over an entry served by 9660srv,
i see this error.

	; bind /386/bin/awk /n/cd0/386/bin/awk
	; du -s /n/cd0/386/bin
	du: /n/cd1/386/bin: seek in directory
	55724	/n/cd1/386/bin

a quick look shows that /sys/src/cmd/9660srv/9660srv.c:346 is the
source of the error and read(5) indicates that this really is an
error.  the bug must be induced by whoever is getting the offset
wrong, since directory offsets must either continue at the previous
offset or 0; seek is not allowed  (note: ramfs does not exhibit this
behavior.)

if i add a little debugging to 9660srv, i get

	du: /n/cd1/386/bin: seek in directory offset 8198 != dp->doffset 8190

and if i add a little more, i find that the closest doffset set
is at the entry "page"

	[PAGE] ip->doffset = offset 0 + rcnt 8190 = 8190

i'm quite puzzled by why binding in an entry (which 9660srv should
not know about) would cause the offset to be wrong.  this seems
like a bug in the kernel.  i'm fighting that conclusion, though.  one
would think that this problem would have reared its head before.

on a bit further inspection of sysfile:/^mountfix, i think the problem
might be that the winning bind spans buffers.

- erik

ps. this workaround, similar to the code in ramfs does "work"
but it contradicts read(5), so it must be wrong.

static long
ireaddir(Xfile *f, uchar *buf, long offset, long count)
{
	Isofile *ip = f->ptr;
	Dir d;
	char names[4*Maxname];
	uchar dbuf[256];
	Drec *drec = (Drec *)dbuf;
	int n, rcnt;

	if(offset==0 || offset != ip->doffset){
		ip->offset = 0;
		ip->doffset = offset;
	}else
		offset = 0;
	rcnt = 0;
	setnames(&d, names);
	while(rcnt < count && getdrec(f, drec) >= 0){
		if(drec->namelen == 1){
			if(drec->name[0] == 0)
				continue;
			if(drec->name[0] == 1)
				continue;
		}
		rzdir(f->xf, &d, ip->fmt, drec);
		d.qid.vers = f->qid.vers;
		if((n = convD2M(&d, buf+rcnt, count-rcnt)) <= BIT16SZ){
			ungetdrec(f);
			break;
		}
		if(offset > 0)
			offset -= n;
		if(offset <= 0)
			rcnt += n;
	}
	ip->doffset += rcnt;
	return rcnt;
}

Re: [9fans] bind vs. 9660srv

[Russ Cox]

This is a kernel bug.
sysfile.c:/^read says

        dir = c->qid.type&QTDIR;
        if(dir && mountrockread(c, p, n, &nn)){
                /* do nothing: mountrockread filled buffer */
        }else{
                if(dir && c->umh)
                        nn = unionread(c, p, n);
                else
                        nn = devtab[c->type]->read(c, p, n, off);
        }
        if(dir)
                nnn = mountfix(c, p, nn, n);
        else
                nnn = nn;

but should say (untested)

        if(c->qid.type&QTDIR) {
                if(mountrockread(c, p, n, &nn)) {
                        /* do nothing: mountrockread filled buffer */
                } else if(c->umh) {
                        nn = unionread(c, p, n);
                } else {
                        if(off != c->offset)
                                error(Edirseek);
                        nn = devtab[c->type]->read(c, p, n, c->devoffset);
                }
                nnn = mountfix(c, p, nn, n);
        } else {
                nn = devtab[c->type]->read(c, p, n, off);
                nnn = nn;
        }

I have separated out the various if(dir) tests into
one block, but more importantly the new code passes
c->devoffset to the directory read.

No one noticed before because most 9P2000 servers
assume they are being used correctly and implement
a simpler check: if offset == 0, seek to beginning,
otherwise continue where the last read left off.

Russ

Re: [9fans] bind vs. 9660srv

[erik quanstrom]

> but should say (untested)

s/un(tested)/\1

> No one noticed before because most 9P2000 servers
> assume they are being used correctly and implement
> a simpler check: if offset == 0, seek to beginning,
> otherwise continue where the last read left off.

ken fs does so i'm still a bit puzzled.

- erik

Re: [9fans] bind vs. 9660srv

[Russ Cox]

On Mon, Aug 10, 2009 at 11:27 AM, erik quanstrom<quanstro@quanstro.net> wrote:
>> but should say (untested)
>
> s/un(tested)/\1
>
>> No one noticed before because most 9P2000 servers
>> assume they are being used correctly and implement
>> a simpler check: if offset == 0, seek to beginning,
>> otherwise continue where the last read left off.
>
> ken fs does so i'm still a bit puzzled.

Not the code I'm looking at
(/sys/src/cmd/cwfs/9p2.c)

	start += n;
	if(start < offset)
		continue;
	if(count < n){
		putbuf(p1);
		goto out1;
	}

There's no check that you get
to start == offset before copying data in.
So if you passed in an offset that was
off by a few bytes, it would get rounded
to a real directory entry boundary.

Russ

Re: [9fans] bind vs. 9660srv

[erik quanstrom]

> Not the code I'm looking at
> (/sys/src/cmd/cwfs/9p2.c)
>
> 	start += n;
> 	if(start < offset)
> 		continue;
> 	if(count < n){
> 		putbuf(p1);
> 		goto out1;
> 	}

i think that it recomputes what the offset should be,
which might work as long as the offsets aren't
too different. and the error has the right sign.
ramfs does something similar, which also happened
to work out in the cases i tested.

from slightly above the point you mention:

dread:
	/*
	 * Pick up where we left off last time if nothing has changed,
	 * otherwise must scan from the beginning.
	 */
	if(offset == file->doffset /*&& file->qid.vers == file->dvers*/){
		addr = file->dslot/DIRPERBUF;
		slot = file->dslot%DIRPERBUF;
		start = offset;
	}
	else{
		addr = 0;
		slot = 0;
		start = 0;
	}

- erik