On Tue, Aug 6, 2024 at 6:17 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Tue, 2024-08-06 at 17:25 +0200, Mateusz Guzik wrote: > > On Tue, Aug 6, 2024 at 4:32 PM Jeff Layton <jlayton@xxxxxxxxxx> > > wrote: > > > > > > Today, when opening a file we'll typically do a fast lookup, but if > > > O_CREAT is set, the kernel always takes the exclusive inode lock. I > > > assume this was done with the expectation that O_CREAT means that > > > we > > > always expect to do the create, but that's often not the case. Many > > > programs set O_CREAT even in scenarios where the file already > > > exists. > > > > > > This patch rearranges the pathwalk-for-open code to also attempt a > > > fast_lookup in certain O_CREAT cases. If a positive dentry is > > > found, the > > > inode_lock can be avoided altogether, and if auditing isn't > > > enabled, it > > > can stay in rcuwalk mode for the last step_into. > > > > > > One notable exception that is hopefully temporary: if we're doing > > > an > > > rcuwalk and auditing is enabled, skip the lookup_fast. Legitimizing > > > the > > > dentry in that case is more expensive than taking the i_rwsem for > > > now. > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > --- > > > Here's a revised patch that does a fast_lookup in the O_CREAT > > > codepath > > > too. The main difference here is that if a positive dentry is found > > > and > > > audit_dummy_context is true, then we keep the walk lazy for the > > > last > > > component, which avoids having to take any locks on the parent > > > (just > > > like with non-O_CREAT opens). > > > > > > The testcase below runs in about 18s on v6.10 (on an 80 CPU > > > machine). > > > With this patch, it runs in about 1s: > > > > > > > I don't have an opinion on the patch. > > > > If your kernel does not use apparmor and the patch manages to dodge > > refing the parent, then indeed this should be fully deserialized just > > like non-creat opens. > > > > Yep. Pity that auditing will slow things down, but them's the breaks... > > > Instead of the hand-rolled benchmark may I interest you in using > > will-it-scale instead? Notably it reports the achieved rate once per > > second, so you can check if there is funky business going on between > > reruns, gives the cpu the time to kick off turbo boost if applicable > > etc. > > > > I would bench with that myself, but I temporarily don't have handy > > access to bigger hw. Even so, the below is completely optional and > > perhaps more of a suggestion for the future :) > > > > I hacked up the test case based on tests/open1.c. > > > > git clone https://github.com/antonblanchard/will-it-scale.git > > > > For example plop into tests/opencreate1.c && gmake && > > ./opencreate1_processes -t 70: > > > > #include <stdlib.h> > > #include <unistd.h> > > #include <sys/types.h> > > #include <sys/stat.h> > > #include <fcntl.h> > > #include <assert.h> > > #include <string.h> > > > > char *testcase_description = "Separate file open/close + O_CREAT"; > > > > #define template "/tmp/willitscale.XXXXXX" > > static char (*tmpfiles)[sizeof(template)]; > > static unsigned long local_nr_tasks; > > > > void testcase_prepare(unsigned long nr_tasks) > > { > > int i; > > tmpfiles = (char(*)[sizeof(template)])malloc(sizeof(template) > > * nr_tasks); > > assert(tmpfiles); > > > > for (i = 0; i < nr_tasks; i++) { > > strcpy(tmpfiles[i], template); > > char *tmpfile = tmpfiles[i]; > > int fd = mkstemp(tmpfile); > > > > assert(fd >= 0); > > close(fd); > > } > > > > local_nr_tasks = nr_tasks; > > } > > > > void testcase(unsigned long long *iterations, unsigned long nr) > > { > > char *tmpfile = tmpfiles[nr]; > > > > while (1) { > > int fd = open(tmpfile, O_RDWR | O_CREAT, 0600); > > assert(fd >= 0); > > close(fd); > > > > (*iterations)++; > > } > > } > > > > void testcase_cleanup(void) > > { > > int i; > > for (i = 0; i < local_nr_tasks; i++) { > > unlink(tmpfiles[i]); > > } > > free(tmpfiles); > > } > > > > > > Good suggestion and thanks for the testcase. With v6.10 kernel, I'm > seeing numbers like this at -t 70: > > min:4873 max:11510 total:418915 > min:4884 max:10598 total:408848 > min:3704 max:12371 total:467658 > min:2842 max:11792 total:418239 > min:2966 max:11511 total:414144 > min:4756 max:11381 total:413137 > min:4557 max:10789 total:404628 > min:4780 max:11125 total:413349 > min:4757 max:11156 total:405963 > > ...with the patched kernel, things are significantly faster: > > min:265865 max:508909 total:21464553 > min:263252 max:500084 total:21242190 > min:263989 max:504929 total:21396968 > min:263343 max:505852 total:21346829 > min:263023 max:507303 total:21410217 > min:263420 max:506593 total:21426307 > min:259556 max:494529 total:20927169 > min:264451 max:508967 total:21433676 > min:263486 max:509460 total:21399874 > min:263906 max:507400 total:21393015 > > I can get some fancier plots if anyone is interested, but the benefit > of this patch seems pretty clear. In the commit message I would replace that handrolled bench thing with +/-: <quote> 70-way open(.., O_RDWR | O_CREAT) calls on already existing files, one per-worker: before: 467658 after: 21464553 (+4589%) </quote> I guess it would make sense to check if unlink1_processes is doing fine if it's not too much hassle. overall pretty decent win :P -- Mateusz Guzik <mjguzik gmail.com>
