Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752960AbbHaMIE (ORCPT ); Mon, 31 Aug 2015 08:08:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44827 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752514AbbHaMIB (ORCPT ); Mon, 31 Aug 2015 08:08:01 -0400 Date: Mon, 31 Aug 2015 14:05:25 +0200 From: Oleg Nesterov To: Eric Dumazet Cc: Al Viro , Linus Torvalds , "linux-kernel@vger.kernel.org" , Andrew Morton , Thomas Gleixner , Ingo Molnar , Maciej =?utf-8?Q?=C5=BBenczykowski?= Subject: change filp_close() to use __fput_sync() ? (Was: [PATCH] task_work: remove fifo ordering guarantee) Message-ID: <20150831120525.GA31015@redhat.com> References: <1440816150.8932.123.camel@edumazet-glaptop2.roam.corp.google.com> <20150829124921.GA14973@redhat.com> <1440856650.8932.144.camel@edumazet-glaptop2.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1440856650.8932.144.camel@edumazet-glaptop2.roam.corp.google.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2532 Lines: 84 On 08/29, Eric Dumazet wrote: > > On Sat, 2015-08-29 at 14:49 +0200, Oleg Nesterov wrote: > > On 08/28, Eric Dumazet wrote: > > > > > > From: Eric Dumazet > > > > > > In commit f341861fb0b ("task_work: add a scheduling point in > > > task_work_run()") I fixed a latency problem adding a cond_resched() > > > call. > > > > > > Later, commit ac3d0da8f329 added yet another loop to reverse a list, > > > bringing back the latency spike : > > > > > > I've seen in some cases this loop taking 275 ms, if for example a > > > process with 2,000,000 files is killed. > > > > > > We could add yet another cond_resched() in the reverse loop, > > > > Can't we do this? > > Well, I stated in the changelog we could. Obviously we can. > > Adding 275 ms of pure overhead to perform this list reversal for files > to be closed is quite unfortunate. Well, if the first loop takes 275 ms, then probably the next one which actually does a lot of __fput's takes much, much more time, so perhaps these 275 ms are not very noticable. Ignoring the latency problem. But of course, this is not good, I agree. Please see below. > > Fifo just looks more sane to me. > > Well, files are closed in a random order. These are the main user of > this stuff. This is the most "heavy" user. But task_works is the generic API. > Now we also could question why we needed commit > 4a9d4b024a3102fc083c925c242d98ac27b1c5f6 ("switch fput to task_work_add > ") since it seems quite an overhead at task exit with 10^6 of files to > close. How about the patch below? I didn't try to test it yet, but since filp_close() does ->flush() I think __fput_sync() should be safe here. Al, what do you think? Oleg. --- x/fs/file_table.c +++ x/fs/file_table.c @@ -292,11 +292,8 @@ void fput(struct file *file) */ void __fput_sync(struct file *file) { - if (atomic_long_dec_and_test(&file->f_count)) { - struct task_struct *task = current; - BUG_ON(!(task->flags & PF_KTHREAD)); + if (atomic_long_dec_and_test(&file->f_count)) __fput(file); - } } EXPORT_SYMBOL(fput); --- x/fs/open.c +++ x/fs/open.c @@ -1074,7 +1074,7 @@ int filp_close(struct file *filp, fl_owner_t id) dnotify_flush(filp, id); locks_remove_posix(filp, id); } - fput(filp); + __fput_sync(filp); return retval; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/