Re: [PATCH 1/2] mm: Make task in balance_dirty_pages() killable

[Jan Kara <jack@suse.cz>]

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 <jack@suse.cz>
SUSE Labs, CR