2011/12/1 Kostik Belousov <kostik...@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 <k...@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 _______________________________________________ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"