Date: Thu, 1 Dec 2011 14:36:28 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, peterjeremy@acm.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Attilio Rao <attilio@freebsd.org>, svn-src-head@freebsd.org, Jeff Roberson <jeff@freebsd.org> Subject: Re: svn commit: r226042 - in head/sys: kern sys Message-ID: <CACfq0914fG6xDgsxjikZwPW4Viwgovtnqpck9DdKEcn9bvGWGQ@mail.gmail.com> In-Reply-To: <20111201114846.GI50300@deviant.kiev.zoral.com.ua> References: <201110051656.p95Gu6Cw020744@svn.freebsd.org> <CACfq090D6CgH=hbLxGwMaBpAnui0ibfSMbqAsBGgXinEJQG7Hg@mail.gmail.com> <20111201114846.GI50300@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/12/1 Kostik Belousov <kostikbel@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@freebsd.org> wr= ote: >> > Author: kib >> > Date: Wed Oct =A05 16:56:06 2011 >> > New Revision: 226042 >> > URL: http://svn.freebsd.org/changeset/base/226042 >> > >> > Log: >> > =A0Supply unique (st_dev, st_ino) value pair for the fstat(2) done on = the pipes. >> > >> > =A0Reviewed by: =A0jhb, Peter Jeremy <peterjeremy acm org> >> > =A0MFC after: =A0 =A02 weeks >> > >> > Modified: >> > =A0head/sys/kern/sys_pipe.c >> > =A0head/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 la= zy >> and alloc a new unr number inside pipe_stat instead of during pipe creat= ion. >> 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 th= e 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 f= ind 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. Th= e > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq0914fG6xDgsxjikZwPW4Viwgovtnqpck9DdKEcn9bvGWGQ>