From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 1/2] mm: Make task in balance_dirty_pages() killable Date: Tue, 15 Nov 2011 15:44:52 +0100 Message-ID: <20111115144452.GA25269@quack.suse.cz> References: <1321287324-15121-1-git-send-email-jack@suse.cz> <1321287324-15121-2-git-send-email-jack@suse.cz> <20111115114844.GA11665@localhost> <20111115134127.GF21732@quack.suse.cz> <20111115141528.GB25196@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , Andrew Morton , Christoph Hellwig , Al Viro , "linux-fsdevel@vger.kernel.org" To: Wu Fengguang Return-path: Received: from cantor2.suse.de ([195.135.220.15]:57396 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755843Ab1KOOpO (ORCPT ); Tue, 15 Nov 2011 09:45:14 -0500 Content-Disposition: inline In-Reply-To: <20111115141528.GB25196@localhost> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 15-11-11 22:15:28, Wu Fengguang wrote: > On Tue, Nov 15, 2011 at 09:41:27PM +0800, Jan Kara wrote: > > On Tue 15-11-11 19:48:44, Wu Fengguang wrote: > > > > +static int balance_dirty_pages(struct address_space *mapping, > > > > unsigned long pages_dirtied) > > > > { > > > > unsigned long nr_reclaimable; /* = file_dirty + unstable_nfs */ > > > > @@ -1020,6 +1023,7 @@ static void balance_dirty_pages(struct address_space *mapping, > > > > unsigned long pos_ratio; > > > > struct backing_dev_info *bdi = mapping->backing_dev_info; > > > > unsigned long start_time = jiffies; > > > > + int err = 0; > > > > > > > > for (;;) { > > > > /* > > > > @@ -1133,7 +1137,7 @@ pause: > > > > pages_dirtied, > > > > pause, > > > > start_time); > > > > - __set_current_state(TASK_UNINTERRUPTIBLE); > > > > + __set_current_state(TASK_KILLABLE); > > > > io_schedule_timeout(pause); > > > > > > > > dirty_thresh = hard_dirty_limit(dirty_thresh); > > > > @@ -1145,6 +1149,11 @@ pause: > > > > */ > > > > if (nr_dirty < dirty_thresh) > > > > break; > > > > + > > > > + if (fatal_signal_pending(current)) { > > > > + err = -EINTR; > > > > + break; > > > > + } > > > > } > > > > > > The other alternative is to raise the limit on fatal_signal_pending: > > > > > > if (fatal_signal_pending(current) && > > > nr_dirty < dirty_thresh + dirty_thresh / 2) > > > break; > > > > > > That should work well enough in practice and avoids touching the fs code. > > Sorry, but I fail to see what would this bring us... Can you elaborate a > > bit please? > > It's not bringing us something, but allows us to get rid of patch 2 > as well as adding fatal_signal_pending() tests to all the other fs. > > So no more worries about partial writes :) > > Sure it leaves ->write_begin() blocks unhandled, however that should > be much less a problem than the blocks inside balance_dirty_pages(). Ah, OK, I see. Frankly, I'd rather keep the loop break in generic_perform_write() and have the code straightforward. I don't see any security (data exposure) problem there and if someone complains on the grounds that the behavior has changed, we can revert the change in the worst case. Honza -- Jan Kara SUSE Labs, CR