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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140315031759.GC22728@two.firstfloor.org>
Date:	Sat, 15 Mar 2014 04:17:59 +0100
From:	Andi Kleen <andi@...stfloor.org>
To:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Tony Luck <tony.luck@...el.com>,
	Wanpeng Li <liwanp@...ux.vnet.ibm.com>,
	Dave Chinner <david@...morbit.com>,
	Jun'ichi Nomura <j-nomura@...jp.nec.com>, linux-mm@...ck.org
Subject: Re: [PATCH 2/6] mm/memory-failure.c: report and recovery for
 memory error on dirty pagecache

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? 

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

> 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

> @@ -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?

>  		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?

>   * 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.

> + * 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? 

> +		 */
> +		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.

-Andi

-- 
ak@...ux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/