Re: [PATCH 2/6] mm/memory-failure.c: report and recovery for memory error on dirty pagecache

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Sat, Mar 15, 2014 at 04:17:59AM +0100, Andi Kleen wrote:
> On Thu, Mar 13, 2014 at 05:39:42PM -0400, Naoya Horiguchi wrote:
> > Unifying error reporting between memory error and normal IO errors is ideal
> > in a long run, but at first let's solve it separately. I hope that some code
> > in this patch will be helpful when thinking of the unification.
> 
> The mechanisms should be very similar, right? 

Yes.

> It may be better to do both at the same time.

Yes, it's better, but it's not trivial to test and confirm that patches
work fine (and I must learn more about IO error.)
But anyway, I'll try this maybe in next post.

> > index 60829565e552..1e8966919044 100644
> > --- v3.14-rc6.orig/include/linux/fs.h
> > +++ v3.14-rc6/include/linux/fs.h
> > @@ -475,6 +475,9 @@ struct block_device {
> >  #define PAGECACHE_TAG_DIRTY	0
> >  #define PAGECACHE_TAG_WRITEBACK	1
> >  #define PAGECACHE_TAG_TOWRITE	2
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +#define PAGECACHE_TAG_HWPOISON	3
> > +#endif
> 
> No need to ifdef defines

OK, I found that if CONFIG_MEMORY_FAILURE is n no one sets/checks this flag,
so it's not problematic that the number of PAGECACHE_TAG_* is more than
RADIX_TREE_MAX_TAGS (3 if !CONFIG_MEMORY_FAILURE). I'll remove this ifdef.

> > @@ -1133,6 +1139,10 @@ static void do_generic_file_read(struct file *filp, loff_t *ppos,
> >  			if (unlikely(page == NULL))
> >  				goto no_cached_page;
> >  		}
> > +		if (unlikely(PageHWPoison(page))) {
> > +			error = -EHWPOISON;
> > +			goto readpage_error;
> > +		}
> 
> Didn't we need this check before independent of the rest of the patch?

I think this check should come with the rest of this patch, because before
this patchset, we have no page with PageHWPoison on pagecache (memory_failure()
removes it from pagecache via me_pagecache_clean(),) so the above check can't
detect error-affected address. Dummy hwpoison page introduced by this patch
makes it detectable.

> >  		if (PageReadahead(page)) {
> >  			page_cache_async_readahead(mapping,
> >  					ra, filp, page,
> > @@ -2100,6 +2110,10 @@ inline int generic_write_checks(struct file *file, loff_t *pos, size_t *count, i
> >          if (unlikely(*pos < 0))
> >                  return -EINVAL;
> >  
> > +	if (unlikely(mapping_hwpoisoned_range(file->f_mapping, *pos,
> > +					      *pos + *count)))
> > +		return -EHWPOISON;
> 
> How expensive is that check? This will happen on every write.
> Can it be somehow combined with the normal page cache lookup?

OK, so it's better to put this check just after a_ops->write_begin in
generic_perform_write(). If we find PageHWPoison, we break the do-while loop,
then we can do write correctly for healthy address before the error address.

> >   * Dirty pagecache page
> > + *
> > + * Memory error reporting (important especially on dirty pagecache error
> > + * because dirty data is lost) with AS_EIO flag has some problems:
> 
> It doesn't make sense to have changelogs in comments. That is what
> git is for.  At some point noone will care about the previous code.

Right, I'll remove this.

> > + * To solve these, we handle dirty pagecache errors by replacing the error
> 
> This part of the comment is good.
> 
> > +	pgoff_t index;
> > +	struct inode *inode = NULL;
> > +	struct page *new;
> >  
> >  	SetPageError(p);
> > -	/* TBD: print more information about the file. */
> >  	if (mapping) {
> > +		index = page_index(p);
> > +		/*
> > +		 * we take inode refcount to keep it's pagecache or mapping
> > +		 * on the memory until the error is resolved.
> 
> How does that work? Who "resolves" the error? 

This comment should have come with patch 3 which adds the resolver.
# I at first wrote a patch with patch 2 and 3 merged and after separated it,
# and my splitting was poor. I'll fix this.

> > +		 */
> > +		inode = igrab(mapping->host);
> > +		pr_info("MCE %#lx: memory error on dirty pagecache (page offset:%lu, inode:%lu, dev:%s)\n",
> 
> Add the word file somewhere, you need to explain this in terms normal
> sysadmins and not only kernel hackers can understand.

OK, so "MCE %#lx: memory error on dirty file cache (page offset:%lu, inode:%lu, dev:%s)\n"
looks better to me.

Thank you very much for close looking.
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>