Re: [PATCH] writeback: add a safety limit to the SIGKILL abort

[Jan Kara <jack@suse.cz>]

On Mon 28-11-11 11:33:40, Wu Fengguang wrote:
> On Mon, Nov 28, 2011 at 11:08:42AM +0800, Wu Fengguang wrote:
> > On Wed, Nov 23, 2011 at 11:06:56PM +0800, Theodore Ts'o wrote:
> > > 
> > > On Nov 23, 2011, at 8:27 AM, Wu Fengguang wrote:
> > > 
> > > > 
> > > > Reading Ted's information feed, I tend to disregard the partial write
> > > > issue: since the "broken" applications will already fail and get
> > > > punished in various other cases, I don't care adding one more penalty
> > > > case to them :-P
> > > 
> > > Just wait until you have a bunch of rabid application programmers,
> > > questioning your judgement, your morality, and even your paternity.
> > > :-)
> > 
> > Ah OK, that sounds frightening. Hmm, till now every one have
> > acknowledged the possibility of data corruption, only that
> > people have different perceptions of the severeness.
> > 
> > Let's rethink things this way: "Is it a _worthwhile_ risk at all?"
> > I'm afraid not. Considering the origin of this patch
> > 
> > [BUG] aborted ext4 leads to inifinity loop in balance_dirty_pages
> > http://www.spinics.net/lists/linux-ext4/msg28464.html
> > 
> > I *think* Jan's first patch is already enough for fixing the bug. IWO
> > the patch we worried/discussed so much is really an optional one. I
> > would imagine the easy and safe solution is to just drop it. Any
> > objections?
> 
> Here is the replacement patch.
> 
> ---
> Subject: writeback: add a safety limit to the SIGKILL abort
> Date: Mon Nov 28 11:16:54 CST 2011
> 
> This adds a safety limit to the SIGKILL abort in commit 499d05ecf990
> ("mm: Make task in balance_dirty_pages() killable"). This will avoid
> dirty pages rushing arbitrarily high in the case some task receives
> SIGKILL and hence become *unthrottled* when doing a huge sized write.
> 
> The alternative way is to check SIGKILL and return partial write in
> generic_perform_write(). However it will lead to data corruption as
> put by Andrew Morton:
> 
>  Previously if an app did write(file, 128k) and was hit with SIGKILL, it
>  would write either 0 bytes or 128k bytes.  Now, it can write 36k bytes,
>  yes?  If the target file consisted of a stream of 128k records then the
>  user will claim, with some justification, that Linux corrupted it.
> 
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
  The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

									Honza

> ---
>  mm/page-writeback.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-next.orig/mm/page-writeback.c	2011-11-28 11:13:58.000000000 +0800
> +++ linux-next/mm/page-writeback.c	2011-11-28 11:16:52.000000000 +0800
> @@ -1136,7 +1136,8 @@ pause:
>  		if (task_ratelimit)
>  			break;
>  
> -		if (fatal_signal_pending(current))
> +		if (fatal_signal_pending(current) &&
> +		    nr_dirty <= dirty_thresh + dirty_thresh / 2)
>  			break;
>  	}
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR