svn commit: r226042 - in head/sys: kern sys

[Giovanni Trematerra]

2011/12/1 Kostik Belousov <kostikbel at gmail.com>:
> On Wed, Nov 30, 2011 at 10:01:12PM +0100, Giovanni Trematerra wrote:
>> On Wed, Oct 5, 2011 at 6:56 PM, Konstantin Belousov <kib at freebsd.org> wrote:
>> > Author: kib
>> > Date: Wed Oct  5 16:56:06 2011
>> > New Revision: 226042
>> > URL: http://svn.freebsd.org/changeset/base/226042
>> >
>> > Log:
>> >  Supply unique (st_dev, st_ino) value pair for the fstat(2) done on the pipes.
>> >
>> >  Reviewed by:  jhb, Peter Jeremy <peterjeremy acm org>
>> >  MFC after:    2 weeks
>> >
>> > Modified:
>> >  head/sys/kern/sys_pipe.c
>> >  head/sys/sys/pipe.h
>> >
>>
>> Hi Konstantin,
>> unfortunately your commit introduces a performance penalty of about 3%
>> documented below in a real workload.
>> I guess that fstat(2) on the pipes is seldom used so we could just be lazy
>> and alloc a new unr number inside pipe_stat instead of during pipe creation.
>> In that case if an application doesn't need fstat(2) on the pipes it
>> won't be charged
>> the cost of alloc_unr/free_unr for the fake inode number.
>> The patch I propose, furthermore, fix a panic in the case alloc_unr
>> failed to allocate
>> a new unr number and return -1. This is because ino_t is unsigned and the test
>> pipe_ino > 0 into pipeclose would be true, calling then free_unr when
>> it hasn't to.
>> The proposed patch was tested with a regression test code that you can find here
>>
>> http://www.trematerra.net/patches/pipe-fstat.c
>>
>> Feel free to add it under tools/regression/pipe/
>>
>> Here the proposed patch:
>>
>> http://www.trematerra.net/patches/lazy_inoalloc.diff
>>

> Thank you for looking at this.
> I committed the test, and the fix for the call to free_unr(-1).

Thank you.

> Regarding the lazy allocation of the inode number, I agree with the idea,
> but have some reservations against the implementation. If several threads
> call fstat(2) on the same pipe which inode is not yet initialized, then
> I see a race in the patch. The easiest workaround is to cover the inode
> allocation with the pipe lock.

Ops my bad. you're right.

>
> Also, I find the return of ENOMEM from fstat(2) somewhat questionable. The
> error code is not documented as allowed for the syscall. I prefer to
> not fail the fstat(2) if lazy allocation failed, but return some fake
> value for inode instead.
>

It seems to me a good compromise. I agree with you that return ENOMEM
from fstat(2)
isn't ideal although Linux does. We should document this behavior in
man page though IMO.
I'll provide a patch to man page as soon as possible if others are no
objections at your
updated patch and if that is ok for you.

Thank you.

--
Gianni