[OpenAFS-devel] Issues with my precache patch (was: Documentation for fs precache)
Phillip Moore
w.phillip.moore@gmail.com
Tue, 19 Oct 2010 13:42:04 -0400
--001485f860949cf1650492fbcff1
Content-Type: text/plain; charset=ISO-8859-1
I took a simple first pass this, by simply allowing "precache" to behave
similar to commands like "bypassthreshold" and "storebehind". This seemed
like it would be a simple matter of copying the approach used by other
functions.
Based on how bypassthreshold is implemented, I tried the function pasted
below, and it simply doesn't work, and I wonder if the issue is in the code
that implements the pioctl call (and I can't even find that....)
The resulting fs binary does two things I don't understand. First, when no
argument is given, I expected the pioctl call to set the value of blob.out,
but instead I get this error:
fs: server or network not responding
If I try to set the value to, say, 100000, then I'm getting a completely
different value back:
Precache size 1101949910
Is this a matter of the pioctl() call not supporting returning the value,
perhaps? I'm guessing that the error occurs because the pioctl() code is
written to expect an input value, and the strange integer value is just
random noise, because the same pioctl() code isn't setting the return value
in blob.out at all.
I'm not familiar with how the pioctl code is organized at all. Can someone
point me to the place that actually handles:
code = pioctl(0, VIOCPRECACHE, &blob, 1);
so I can figure this out?
Thanks....
----
static int
PreCacheCmd(struct cmd_syndesc *as, void *arock)
{
afs_int32 code;
struct ViceIoctl blob;
afs_int32 precache_i, precache_o;
if (as->parms[0].items) {
code = util_GetInt32(as->parms[0].items->data, &precache_i);
if (code) {
fprintf(stderr, "%s: bad integer specified for precache size.\n",
pn);
return 1;
}
blob.in = (char *)&precache_i;
blob.in_size = sizeof(precache_i);
} else {
blob.in = NULL;
blob.in_size = 0;
}
blob.out = (char *)&precache_o;
blob.out_size = sizeof(precache_o);
code = pioctl(0, VIOCPRECACHE, &blob, 1);
if (code) {
Die(errno, NULL);
return 1;
}
printf("Precache size %d\n", precache_o);
return 0;
}
On Tue, Oct 19, 2010 at 8:52 AM, Derrick Brashear <shadow@gmail.com> wrote:
> On Tue, Oct 19, 2010 at 7:27 AM, Phillip Moore
> <w.phillip.moore@gmail.com> wrote:
> >
> > Would you accept a patch that introduces two new fs commands:
> >
> > fs getprecache
> > fs setprecache
> > and that "hides" the precache command? IOW, precache will work, but it
> will
> > not show up in fs help output. This introduces a new pair of get/set
> > commands, and would give admins the ability to query the value, and
> > therefore manage it.
> > Alternately, fs precache can be modified to display the value when no
> > arguments are provided, instead of generate a syntax error like it does
> > right now.
>
> Either is fine. A hidden deprecated command might be better in 1.6
> (documented as existing but deprecated) and the master can simply not
> have the command at all so that when 1.next hits, ti's already gone.
>
> > There's a lack of consistency in the fs commands, but my personal
> preference
> > is for get/set commands when we add new features, as opposed to a single
> > command for both setting and querying something.
> > On Mon, Oct 18, 2010 at 1:50 PM, Derrick Brashear <shadow@gmail.com>
> wrote:
> >>
> >> On Mon, Oct 18, 2010 at 1:45 PM, Phillip Moore
> >> <w.phillip.moore@gmail.com> wrote:
> >> > Another fs command I can't find any documentation for: fs precache
> >> > In this case, it appears that there's no way to query the value.
> This
> >> > seems like bad interface design to me. If there's a mechanism for
> >> > setting
> >> > an important value that changes the behavior of the client, there has
> to
> >> > be
> >> > a mechanism for querying that value. Otherwise, you can't manage it.
> >> > Write-only, read-never parameters are very bad.
> >> > Looking at the code in src/venus/fs.c, it looks to me like this
> *should*
> >> > have been implemented as a pair of CLI commands: setprecache and
> >> > getprecache, and in fact, that should be straight forward, and fully
> >> > backwards compatible.
> >>
> >> Probably. And I take blame for that.
> >>
> >> > Is this another bleeding edge feature that the authors thought wasn't
> >> > important enough to write a man page for?
> >>
> >> No.
> >>
> >>
> >>
> >> --
> >> Derrick
> >
> >
>
>
>
> --
> Derrick
>
--001485f860949cf1650492fbcff1
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
I took a simple first pass this, by simply allowing "precache" to=
behave similar to commands like "bypassthreshold" and "stor=
ebehind". =A0 This seemed like it would be a simple matter of copying =
the approach used by other functions.<div>
<br></div><div>Based on how bypassthreshold is implemented, I tried the fun=
ction pasted below, and it simply doesn't work, and I wonder if the iss=
ue is in the code that implements the pioctl call (and I can't even fin=
d that....)</div>
<div><br></div><div>The resulting fs binary does two things I don't und=
erstand. =A0First, when no argument is given, I expected the pioctl call to=
set the value of blob.out, but instead I get this error:</div><div><br></d=
iv>
<div><div>fs: server or network not responding</div></div><div><br></div><d=
iv>If I try to set the value to, say, 100000, then I'm getting a comple=
tely different value back:</div><div><br></div><div><div>Precache size 1101=
949910</div>
</div><div><br></div><div>Is this a matter of the pioctl() call not support=
ing returning the value, perhaps? =A0I'm guessing that the error occurs=
because the pioctl() code is written to expect an input value, and the str=
ange integer value is just random noise, because the same pioctl() code isn=
't setting the return value in blob.out at all.</div>
<div><br></div><div>I'm not familiar with how the pioctl code is organi=
zed at all. =A0Can someone point me to the place that actually handles:</di=
v><div><br></div><div><div>=A0=A0 =A0code =3D pioctl(0, VIOCPRECACHE, &=
blob, 1);</div>
</div><div><br></div><div>so I can figure this out?</div><div><br></div><di=
v>Thanks....</div><div><br></div><div>----</div><div><br></div><div><div>st=
atic int</div><div>PreCacheCmd(struct cmd_syndesc *as, void *arock)</div>
<div>{</div><div>=A0=A0 =A0afs_int32 code;</div><div>=A0=A0 =A0struct ViceI=
octl blob;</div><div>=A0=A0 =A0afs_int32 precache_i, precache_o;</div><div>=
<br></div><div>=A0=A0 =A0if (as->parms[0].items) {</div><div><span class=
=3D"Apple-tab-span" style=3D"white-space:pre"> </span>code =3D util_GetInt3=
2(as->parms[0].items->data, &precache_i);</div>
<div><span class=3D"Apple-tab-span" style=3D"white-space:pre"> </span>if (c=
ode) {</div><div><span class=3D"Apple-tab-span" style=3D"white-space:pre"> =
</span> =A0 =A0fprintf(stderr, "%s: bad integer specified for precache=
size.\n",</div>
<div><span class=3D"Apple-tab-span" style=3D"white-space:pre"> </span> =A0=
=A0pn);</div><div><span class=3D"Apple-tab-span" style=3D"white-space:pre"=
> </span> =A0 =A0return 1;</div><div><span class=3D"Apple-tab-span" style=
=3D"white-space:pre"> </span>}</div>
<div>=A0=A0 =A0 =A0 =A0<a href=3D"http://blob.in">blob.in</a> =3D (char *)&=
amp;precache_i;</div><div>=A0=A0 =A0 =A0 =A0blob.in_size =3D sizeof(precach=
e_i);</div><div>=A0=A0 =A0} else {</div><div>=A0=A0 =A0 =A0 =A0<a href=3D"h=
ttp://blob.in">blob.in</a> =3D NULL;</div>
<div>=A0=A0 =A0 =A0 =A0blob.in_size =3D 0;</div><div>=A0=A0 =A0}</div><div>=
<br></div><div>=A0=A0 =A0blob.out =3D (char *)&precache_o;</div><div>=
=A0=A0 =A0blob.out_size =3D sizeof(precache_o);</div><div>=A0=A0 =A0code =
=3D pioctl(0, VIOCPRECACHE, &blob, 1);</div>
<div>=A0=A0 =A0if (code) {</div><div><span class=3D"Apple-tab-span" style=
=3D"white-space:pre"> </span>Die(errno, NULL);</div><div><span class=3D"App=
le-tab-span" style=3D"white-space:pre"> </span>return 1;</div><div>=A0=A0 =
=A0}</div><div><br>
</div><div>=A0=A0 =A0printf("Precache size %d\n", precache_o);</d=
iv><div>=A0=A0 =A0return 0;</div><div>}</div><div><br></div><br><div class=
=3D"gmail_quote">On Tue, Oct 19, 2010 at 8:52 AM, Derrick Brashear <span di=
r=3D"ltr"><<a href=3D"mailto:shadow@gmail.com">shadow@gmail.com</a>><=
/span> wrote:<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex;">On Tue, Oct 19, 2010 at 7:27 AM, Phillip Mo=
ore<br>
<div class=3D"im"><<a href=3D"mailto:w.phillip.moore@gmail.com">w.philli=
p.moore@gmail.com</a>> wrote:<br>
><br>
</div><div class=3D"im">> Would you accept a patch that introduces two n=
ew fs commands:<br>
><br>
> =A0=A0 =A0fs getprecache<br>
> =A0=A0 =A0fs setprecache<br>
> and that "hides" the precache command? =A0IOW, precache will=
work, but it will<br>
> not show up in fs help output. =A0This introduces a new pair of get/se=
t<br>
> commands, and would give admins the ability to query the value, and<br=
>
> therefore manage it.<br>
> Alternately, fs precache can be modified to display the value when no<=
br>
> arguments are provided, instead of generate a syntax error like it doe=
s<br>
> right now.<br>
<br>
</div>Either is fine. A hidden deprecated command might be better in 1.6<br=
>
(documented as existing but deprecated) and the master can simply not<br>
have the command at all so that when 1.next hits, ti's already gone.<br=
>
<div class=3D"im"><br>
> There's a lack of consistency in the fs commands, but my personal =
preference<br>
> is for get/set commands when we add new features, as opposed to a sing=
le<br>
> command for both setting and querying something.<br>
> On Mon, Oct 18, 2010 at 1:50 PM, Derrick Brashear <<a href=3D"mailt=
o:shadow@gmail.com">shadow@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Oct 18, 2010 at 1:45 PM, Phillip Moore<br>
>> <<a href=3D"mailto:w.phillip.moore@gmail.com">w.phillip.moore@g=
mail.com</a>> wrote:<br>
>> > Another fs command I can't find any documentation for: =
=A0fs precache<br>
>> > In this case, it appears that there's no way to query the=
value. =A0 This<br>
>> > seems like bad interface design to me. =A0If there's a me=
chanism for<br>
>> > setting<br>
>> > an important value that changes the behavior of the client, t=
here has to<br>
>> > be<br>
>> > a mechanism for querying that value. =A0Otherwise, you can=
9;t manage it.<br>
>> > =A0Write-only, read-never parameters are very bad.<br>
>> > Looking at the code in src/venus/fs.c, it looks to me like th=
is *should*<br>
>> > have been implemented as a pair of CLI commands: setprecache =
and<br>
>> > getprecache, and in fact, that should be straight forward, an=
d fully<br>
>> > backwards compatible.<br>
>><br>
>> Probably. And I take blame for that.<br>
>><br>
>> > Is this another bleeding edge feature that the authors though=
t wasn't<br>
>> > important enough to write a man page for?<br>
>><br>
>> No.<br>
>><br>
>><br>
>><br>
>> --<br>
>> Derrick<br>
><br>
><br>
<br>
<br>
<br>
</div>--<br>
<font color=3D"#888888">Derrick<br>
</font></blockquote></div><br></div>
--001485f860949cf1650492fbcff1--