[OpenAFS-devel] leaking a Fid ref equals doom? (was Re: [OpenAFS] vos release problem)

[Derrick J Brashear]

Moved here from openafs-info, since this is a development question now.

> could someone please review the following problem
> (OpenAFS 1.2.10, i386_linux24):
>
> - create a volume at fileserver A (vos create)
> - mount it and create a file within the volume (touch)
> - define fileserver A as replica site (vos addsite)
> - define fileserver B as a second replica site (vos addsite)
> - create the replicas (vos release)
> - create a second file within the volume (e.g. modify the content of
>   the volume's root directory - touch)
> - re-create the replicas (vos release)
> - look into the RO volume, make sure to access fileserver B by setting
>   the fileserver preferences
> - my problem at this point is: the volume's root directory is empty

So what happens is this: in ViceCreateRoot, we end up with an outstanding
IHandle ref from the call to Length (the other calls which get one either
drop it or have a corresponding call to drop it, e.g. DZap for MakeDir,
FidZap for SetSalvageDirHandle). Length calls DRead, which calls newslot
if we don't already have a buffer. We don't, so since we need one, when
we finish creating the volume root, we still have a fid ref (including an
ihandle) from the buffer in the dir package's buffer cache. (We got it
from a call to FidCpy, and it's not returned to the caller in DRead,
because it's basically internal to the dir package)

That's fine, but later during the "Restore" of the volume dump we got from
the network, we unlink the file behind this since we create a new file
to hold the same vnode number (1). So, now we have a ref'd IHandle
attached to an unlinked file.

Ok, now you change the rw and release the volume again. Another dump comes
down the pipe, and you reuse the filename you unlinked before, because of
how the namei server allocates tags. You create a new file. When you
ih_open it, you see, hey, i have a handle for this pathname already, so
you reuse it, write the directory into it, and then have nothing in the
file you just created. Boom.

So I think the correct "fix" is when you receive a volume dump to restore,
you should call DFlushVolume on the volume you're restoring to flush any
dir buffers you have.

However, in the interest of safety, namei_idec should call FDH_REALLYCLOSE
at the unlink (in the else), *or* namei_icreate should do the same when
it calls IH_OPEN(lh) (FDH_REALLYCLOSE and then IH_OPEN again), *or* both.

I'm pretty sure the "fix" is the one we want; I'm less sure about the
safety measures. I'm leaning toward protection only in icreate. There may
be a way to optimize it to not use 2 open()s; I suppose looking for an
ih_refcnt of >=2 (as opposed to 1) is a sure danger sign and could be used
to determine if we should worry, and otherwise don't REALLYCLOSE and OPEN
again.

Comments?

(Suggested "fix" would be adding:
DFlushVolume(V_parentId(tt->volume));
just before the call to RestoreVolume in src/volser/volprocs.c)