Discussion:
vn_setpath
Andrew Deason
2013-01-04 19:55:48 UTC
Permalink
Solaris 11.1 appears to have gained a new vn_setpath argument. Before,
in Solaris 11, we had:

void vn_setpath(vnode_t *rootvp, struct vnode *startvp, struct vnode *vp,
const char *path, size_t plen);

And now there is:

void vn_setpath(vnode_t *rootvp, struct vnode *startvp, struct vnode *vp,
const char *path, size_t plen, boolean_t force);

I can't find any information on what this is. Could we get a hint as to
what this is for, or what it does, ...? I mean, I assume it somehow
'forces' setting the path for the vnode, but I have no idea what the
effects are, why I would set or clear it, etc.

Also, does this mean that a kernel module compiled for Solaris 11 will
have undefined behavior wrt this on a Solaris 11.1 machine? I tried
loading such a module, and it seems to work fine, but I don't know if
there's a problem lurking.
--
Andrew Deason
***@sinenomine.net
Frank Batschulat (private)
2013-01-08 18:01:51 UTC
Permalink
Post by Andrew Deason
Solaris 11.1 appears to have gained a new vn_setpath argument. Before,
void vn_setpath(vnode_t *rootvp, struct vnode *startvp, struct vnode *vp,
const char *path, size_t plen);
void vn_setpath(vnode_t *rootvp, struct vnode *startvp, struct vnode *vp,
const char *path, size_t plen, boolean_t force);
I can't find any information on what this is. Could we get a hint as to
what this is for, or what it does, ...? I mean, I assume it somehow
'forces' setting the path for the vnode, but I have no idea what the
effects are, why I would set or clear it, etc.
Also, does this mean that a kernel module compiled for Solaris 11 will
have undefined behavior wrt this on a Solaris 11.1 machine? I tried
loading such a module, and it seems to work fine, but I don't know if
there's a problem lurking.
Hey Andrew, you caught us by surprise here.

We've discussed your issue, and I must admit we've not anticipated some file system
implementations making use of vn_setpath(). looking at the one occurrence of
vn_setpath() usage in OpenAFS its not much surprising to see it being placed in gafs_rename(),
the VOP_RENAME() implementation. Usually vn_setpath() is only expected to be called by
the pathname resolution and lookup code in the kernel. I suppose your code pre-dates
the introduction of vn_renamepath() which is what our file system implementations actually
use to deal with the update of the vnodes path after a successful rename.

http://git.openafs.org/?p=openafs.git;a=blob;f=src/afs/SOLARIS/osi_vnodeops.c

so in gasf_rename() we have:

1561 mutex_enter(&vp->v_lock);
1562 if (vp->v_path != NULL) {
1563 kmem_free(vp->v_path, strlen(vp->v_path) + 1);
1564 vp->v_path = NULL;
1565 }
1566 mutex_exit(&vp->v_lock);
1567 vn_setpath(afs_globalVp, pvp, vp, aname2, strlen(aname2));

As far as a possible impact of a AFS module compiled on Solaris 11 without the force flag
to vn_setpath() goes, it should be innocuous on Solaris 11 Update one and onwards.
It'll possibly just replace the existing path.

However, the way the above code is in gasf_rename() today, ie, freeing the existing
vnode path upfront before calling vn_setpath(), vn_setpath() will always set v_path anyways
regardless of whether the force flag is true or false. So no harm will happen with the current
compiled modules.

For Solaris 11 (as of build 94 and as for Solaris 10 since update 8) vn_renamepath()
is available and should be used instead, there's no reason to call vn_setpath() other then
indirectly thru vn_renamepath().

eg. a patch similar to this should be done:

--- ./SOLARIS/osi_vnodeops.c.org Mon Dec 10 00:31:37 2012
+++ ./SOLARIS/osi_vnodeops.c Mon Jan 7 13:49:05 2013
@@ -1558,6 +1558,9 @@
if (avcp) {
struct vnode *vp = AFSTOV(avcp), *pvp = AFSTOV(andp);

+#if defined(AFS_SUN511_ENV)
+ vn_renamepath(pvp, vp, aname2, strlen(aname2));
+#else
mutex_enter(&vp->v_lock);
if (vp->v_path != NULL) {
kmem_free(vp->v_path, strlen(vp->v_path) + 1);
@@ -1565,6 +1568,7 @@
}
mutex_exit(&vp->v_lock);
vn_setpath(afs_globalVp, pvp, vp, aname2, strlen(aname2));
+#endif

AFS_RELE(avcp);
}

Btw, we've noticed something else you want to fix in afs_inactive():

1674 /*
1675 * Solaris calls VOP_OPEN on exec, but doesn't call VOP_CLOSE when
1676 * the executable exits. So we clean up the open count here.
1677 *
1678 * Only do this for mvstat 0 vnodes: when using fakestat, we can't
1679 * lose the open count for volume roots (mvstat 2), even though they
1680 * will get VOP_INACTIVE'd when released by afs_PutFakeStat().
1681 */

We have fixed the overall VOP_CLOSE() is missing in the exec path problem since Solaris 11 build 135.
This code above should be disabled in Solaris 11 and later. VOP_CLOSE() is called on exec (on the old executable),
and exit and we also call VOP_OPEN on fork(). However it is not fixed in Solaris 10.

hth, and a happy and successful year 2013 to the OpenAFS world.

cheers
frankB
Andrew Deason
2013-01-08 18:40:31 UTC
Permalink
On Tue, 08 Jan 2013 19:01:51 +0100
Post by Frank Batschulat (private)
We've discussed your issue, and I must admit we've not anticipated
some file system implementations making use of vn_setpath(). looking
at the one occurrence of vn_setpath() usage in OpenAFS its not much
surprising to see it being placed in gafs_rename(), the VOP_RENAME()
implementation. Usually vn_setpath() is only expected to be called by
the pathname resolution and lookup code in the kernel. I suppose your
code pre-dates the introduction of vn_renamepath() which is what our
file system implementations actually use to deal with the update of
the vnodes path after a successful rename.
If it only existed as of Solaris 10u8... yes, we predate it by quite a
lot!
Post by Frank Batschulat (private)
However, the way the above code is in gasf_rename() today, ie, freeing
the existing vnode path upfront before calling vn_setpath(),
vn_setpath() will always set v_path anyways regardless of whether the
force flag is true or false. So no harm will happen with the current
compiled modules.
Okay, thanks, that is good to hear.
Post by Frank Batschulat (private)
+#if defined(AFS_SUN511_ENV)
+ vn_renamepath(pvp, vp, aname2, strlen(aname2));
+#else
We usually do this kind of thing by testing directly for the existence
of the desired function. There is no problem with using this as soon as
it appears, correct?
Post by Frank Batschulat (private)
1674 /*
1675 * Solaris calls VOP_OPEN on exec, but doesn't call VOP_CLOSE when
1676 * the executable exits. So we clean up the open count here.
1677 *
1678 * Only do this for mvstat 0 vnodes: when using fakestat, we can't
1679 * lose the open count for volume roots (mvstat 2), even though they
1680 * will get VOP_INACTIVE'd when released by afs_PutFakeStat().
1681 */
We have fixed the overall VOP_CLOSE() is missing in the exec path
problem since Solaris 11 build 135. This code above should be
disabled in Solaris 11 and later. VOP_CLOSE() is called on exec (on
the old executable), and exit and we also call VOP_OPEN on fork().
However it is not fixed in Solaris 10.
That build number does not mean a lot to me; does that mean it's in
every public Solaris 11 release? I would be careful with disabling it,
since that's not really something we can easily write a direct test for.
And yes, thank you!

By the way, are you okay with being credited in patches for these
things? I would like to note where I got this information from when I
say we should do X, Y, or Z in e.g. commit messages.
--
Andrew Deason
***@sinenomine.net
Frank Batschulat (private)
2013-01-08 22:07:29 UTC
Permalink
Post by Andrew Deason
Post by Frank Batschulat (private)
+#if defined(AFS_SUN511_ENV)
+ vn_renamepath(pvp, vp, aname2, strlen(aname2));
+#else
We usually do this kind of thing by testing directly for the existence
of the desired function. There is no problem with using this as soon as
it appears, correct?
that is correct.
Post by Andrew Deason
Post by Frank Batschulat (private)
1674 /*
1675 * Solaris calls VOP_OPEN on exec, but doesn't call VOP_CLOSE when
1676 * the executable exits. So we clean up the open count here.
1677 *
1678 * Only do this for mvstat 0 vnodes: when using fakestat, we can't
1679 * lose the open count for volume roots (mvstat 2), even though they
1680 * will get VOP_INACTIVE'd when released by afs_PutFakeStat().
1681 */
We have fixed the overall VOP_CLOSE() is missing in the exec path
problem since Solaris 11 build 135. This code above should be
disabled in Solaris 11 and later. VOP_CLOSE() is called on exec (on
the old executable), and exit and we also call VOP_OPEN on fork().
However it is not fixed in Solaris 10.
That build number does not mean a lot to me; does that mean it's in
every public Solaris 11 release?
yes it is, build 135 pre-dates Solaris 11 GA by quite a bit, its even in
the OpenSolaris code base before the "closure"

http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/fs/vnode.c#3022

so you are good to go for Openindiana/Illumos as well in case you care about them.
Post by Andrew Deason
I would be careful with disabling it,
since that's not really something we can easily write a direct test for.
your call, we thought this may lead you astray and confuse the bean counting here
when we now call VOP_CLOSE() in Solaris 11 when you don't expect us todo so. you are in a much
better position to make that judgement actually.
Post by Andrew Deason
things? I would like to note where I got this information from when I
say we should do X, Y, or Z in e.g. commit messages.
yeah, we can probably do that, wouldn't be the first time anyways.

cheers
frankB
Frank Batschulat (private)
2013-01-08 22:13:53 UTC
Permalink
Post by Frank Batschulat (private)
Post by Andrew Deason
Post by Frank Batschulat (private)
1674 /*
1675 * Solaris calls VOP_OPEN on exec, but doesn't call VOP_CLOSE when
1676 * the executable exits. So we clean up the open count here.
1677 *
1678 * Only do this for mvstat 0 vnodes: when using fakestat, we can't
1679 * lose the open count for volume roots (mvstat 2), even though they
1680 * will get VOP_INACTIVE'd when released by afs_PutFakeStat().
1681 */
We have fixed the overall VOP_CLOSE() is missing in the exec path
problem since Solaris 11 build 135. This code above should be
disabled in Solaris 11 and later. VOP_CLOSE() is called on exec (on
the old executable), and exit and we also call VOP_OPEN on fork().
However it is not fixed in Solaris 10.
That build number does not mean a lot to me; does that mean it's in
every public Solaris 11 release?
yes it is, build 135 pre-dates Solaris 11 GA by quite a bit, its even in
the OpenSolaris code base before the "closure"
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/fs/vnode.c#3022
so you are good to go for Openindiana/Illumos as well in case you care about them.
oh I just noticed this was for the vn_renamepath() above in the context of the VOP_CLOSE()
question. for VOP_CLOSE() see:

http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/os/exec.c#533

http://src.opensolaris.org/source/history/onnv/onnv-gate/usr/src/uts/common/os/exec.c

11736:63a134e1f09c 22-Feb-2010 Donghai Qiao 4492533 Filesystems may need VOP_CLOSE() for executables following a VOP_OPEN()

cheers
frankB
Andrew Deason
2013-01-14 21:09:29 UTC
Permalink
On Tue, 08 Jan 2013 23:07:29 +0100
Post by Frank Batschulat (private)
Post by Andrew Deason
I would be careful with disabling it,
since that's not really something we can easily write a direct test for.
your call, we thought this may lead you astray and confuse the bean counting here
when we now call VOP_CLOSE() in Solaris 11 when you don't expect us todo so. you are in a much
better position to make that judgement actually.
Just so you and anyone reading this list is aware, we basically
implemented both of your suggestions. For reference, they are gerrit
8894 and 8895. Thanks again, Frank!
--
Andrew Deason
***@sinenomine.net
Andrew Deason
2013-01-23 19:48:46 UTC
Permalink
On Tue, 08 Jan 2013 23:07:29 +0100
On Tue, 08 Jan 2013 19:40:31 +0100, Andrew Deason
Post by Andrew Deason
Post by Frank Batschulat (private)
+#if defined(AFS_SUN511_ENV)
+ vn_renamepath(pvp, vp, aname2, strlen(aname2));
+#else
We usually do this kind of thing by testing directly for the
existence of the desired function. There is no problem with using
this as soon as it appears, correct?
that is correct.
A small note on this for posterity... originally I did just do this for
SUN511_ENV, since it was easier with the existing autoconf stuff we had
for Solaris. But we got a report of breakage for the same reason on
Solaris 10; the change to vn_setpath appears to also have been
introduced in some patch for Solaris 10. So now we detect vn_renamepath
directly again, and use it wherever it's available.
--
Andrew Deason
***@sinenomine.net
Loading...