Re: [PATCH 2/2] fs: Make write(2) interruptible by a signal

[Jan Kara <jack@suse.cz>]

On Mon 14-11-11 11:26:00, Christoph Hellwig wrote:
> On Mon, Nov 14, 2011 at 05:15:24PM +0100, Jan Kara wrote:
> > Currently write(2) to a file is not interruptible by a signal. Sometimes this
> > is desirable (e.g. when you want to quickly kill a process hogging your disk or
> > when some process gets blocked in balance_dirty_pages() indefinitely due to a
> > filesystem being in an error condition).
> > 
> > Reported-by: Kazuya Mio <k-mio@sx.jp.nec.com>
> > Tested-by: Kazuya Mio <k-mio@sx.jp.nec.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  mm/filemap.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index c0018f2..166b30e 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2407,7 +2407,6 @@ static ssize_t generic_perform_write(struct file *file,
> >  						iov_iter_count(i));
> >  
> >  again:
> > -
> >  		/*
> >  		 * Bring in the user page that we will copy from _first_.
> >  		 * Otherwise there's a nasty deadlock on copying from the
> > @@ -2463,7 +2462,15 @@ again:
> >  		written += copied;
> >  
> >  		balance_dirty_pages_ratelimited(mapping);
> > -
> > +		/*
> > +		 * We check the signal independently of balance_dirty_pages()
> > +		 * because we need not wait and check for signal there although
> > +		 * this loop could have taken significant amount of time...
> > +		 */
> > +		if (fatal_signal_pending(current)) {
> > +			status = -EINTR;
> > +			break;
> > +		}
> 
> Hmm.  If we need to check again every time adding the return value to
> balance_dirty_pages is rather pointless.
> 
> I have a bit of a problem parsing the comment - does it try to say that
> we might spend too much time after the fatal_signal_pending in the
> balance_dirty_pages code so that we have to check it again?  Why not
> repeat the check at the end of balance_dirty_pages_ratelimited and thus
> avoid having to duplicate the thing in all callers?
  The point I was trying to get across is that all:
iov_iter_fault_in_readable()
->write_begin()
...
->write_end()

  can take a rather long time. Considering that
balance_dirty_pages_ratelimited() needn't call balance_dirty_pages() or
balance_dirty_pages() can just return without doing anything because there
aren't many dirty pages, the end result is that
balance_dirty_pages_ratelimited() won't return EINTR for rather long time
although there is a signal pending. That's why I want to check for signal
pending directly in the loop in generic_perform_write().

There can be loops where the only blocking point is in
balance_dirty_pages() and then using the return value makes sense but
I think such loops would be in minority so maybe the return value doesn't
make much sense after all.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR