Re: [PATCH 3/3] HWPOISON: prevent inode cache removal to keep AS_HWPOISON sticky

[Naoya Horiguchi]

On Thu, Aug 23, 2012 at 05:11:25PM +0800, Fengguang Wu wrote:
> On Wed, Aug 22, 2012 at 11:17:35AM -0400, Naoya Horiguchi wrote:
> > "HWPOISON: report sticky EIO for poisoned file" still has a corner case
> > where we have possibilities of data lost. This is because in this fix
> > AS_HWPOISON is cleared when the inode cache is dropped.
> > 
> > For example, consider an application in which a process periodically
> > (every 10 minutes) writes some logs on a file (and closes it after
> > each writes,) and at the end of each day some batch programs run using
> > the log file. If a memory error hits on dirty pagecache of this log file
> > just after periodic write/close and the inode cache is cleared before the
> > next write, then this application is not aware of the error and the batch
> > programs will work wrongly.
> > 
> > To avoid this, this patch makes us pin the hwpoisoned inode on memory
> > until we remove or completely truncate the hwpoisoned file.
> 
> Good point!
>  
> > Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> > ---
> >  fs/inode.c              | 12 ++++++++++++
> >  include/linux/pagemap.h | 11 +++++++++++
> >  mm/memory-failure.c     |  2 +-
> >  mm/truncate.c           |  2 ++
> >  4 files changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git v3.6-rc1.orig/fs/inode.c v3.6-rc1/fs/inode.c
> > index ac8d904..8742397 100644
> > --- v3.6-rc1.orig/fs/inode.c
> > +++ v3.6-rc1/fs/inode.c
> > @@ -717,6 +717,15 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
> >  		}
> >  
> >  		/*
> > +		 * Keep inode caches on memory for user processes to certainly
> > +		 * be aware of memory errors.
> > +		 */
> > +		if (unlikely(mapping_hwpoison(inode->i_mapping))) {
> > +			spin_unlock(&inode->i_lock);
> > +			continue;
> > +		}
> 
> That chunk prevents reclaiming all the cached pages. However the intention
> is only to keep the struct inode together with the hwpoison bit?

Yes, we can not reclaim pagecaches from shrink_slab(), but we can do from
shrink_zone(). So it shouldn't happen that cached pages on hwpoisoned file
remain for long under high memory pressure.

> > +		/*
> >  		 * Referenced or dirty inodes are still in use. Give them
> >  		 * another pass through the LRU as we canot reclaim them now.
> >  		 */
> > @@ -1405,6 +1414,9 @@ static void iput_final(struct inode *inode)
> >  		inode->i_state &= ~I_WILL_FREE;
> >  	}
> >  
> > +	if (unlikely(mapping_hwpoison(inode->i_mapping) && drop))
> > +		mapping_clear_hwpoison(inode->i_mapping);
> 
> Is that clear necessary? Because the bit will be gone with the inode
> struct: it's going to be de-allocated anyway.

With the chunk in prune_icache_sb() we keep the inode struct with
AS_HWPOISON set on memory, so in order to remove it, we need explicitly
clear the bit.
Without this clear, the inode remains until system reboot.

> >  	inode->i_state |= I_FREEING;
> >  	if (!list_empty(&inode->i_lru))
> >  		inode_lru_list_del(inode);
> > diff --git v3.6-rc1.orig/include/linux/pagemap.h v3.6-rc1/include/linux/pagemap.h
> > index 4d8d821..9fce4e4 100644
> > --- v3.6-rc1.orig/include/linux/pagemap.h
> > +++ v3.6-rc1/include/linux/pagemap.h
> > @@ -59,11 +59,22 @@ static inline int mapping_hwpoison(struct address_space *mapping)
> >  {
> >  	return test_bit(AS_HWPOISON, &mapping->flags);
> >  }
> > +static inline void mapping_set_hwpoison(struct address_space *mapping)
> > +{
> > +	set_bit(AS_HWPOISON, &mapping->flags);
> > +}
> > +static inline void mapping_clear_hwpoison(struct address_space *mapping)
> > +{
> > +	clear_bit(AS_HWPOISON, &mapping->flags);
> > +}
> >  #else
> >  static inline int mapping_hwpoison(struct address_space *mapping)
> >  {
> >  	return 0;
> >  }
> > +static inline void mapping_clear_hwpoison(struct address_space *mapping)
> > +{
> > +}
> >  #endif
> >  
> >  static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> > diff --git v3.6-rc1.orig/mm/memory-failure.c v3.6-rc1/mm/memory-failure.c
> > index a1e7e00..ca064c6 100644
> > --- v3.6-rc1.orig/mm/memory-failure.c
> > +++ v3.6-rc1/mm/memory-failure.c
> > @@ -652,7 +652,7 @@ static int me_pagecache_dirty(struct page *p, unsigned long pfn)
> >  		 * the first EIO, but we're not worse than other parts
> >  		 * of the kernel.
> >  		 */
> > -		set_bit(AS_HWPOISON, &mapping->flags);
> > +		mapping_set_hwpoison(mapping);
> >  	}
> >  
> >  	return me_pagecache_clean(p, pfn);
> > diff --git v3.6-rc1.orig/mm/truncate.c v3.6-rc1/mm/truncate.c
> > index 75801ac..82a994f 100644
> > --- v3.6-rc1.orig/mm/truncate.c
> > +++ v3.6-rc1/mm/truncate.c
> > @@ -574,6 +574,8 @@ void truncate_setsize(struct inode *inode, loff_t newsize)
> >  
> >  	oldsize = inode->i_size;
> >  	i_size_write(inode, newsize);
> > +	if (unlikely(mapping_hwpoison(inode->i_mapping) && !newsize))
> 
> It might be a bit better to test !newsize first.

Ah, OK.

Thanks,
Naoya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/