[Cluster-devel] [patch 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

[Cluster-devel] [patch 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

[Jan Kara]

On Tue 24-08-10 03:50:19, David Rientjes wrote:
> Add kmalloc_nofail(), kcalloc_nofail(), and kzalloc_nofail().  These
> functions are equivalent to kmalloc(), kcalloc(), and kzalloc(),
> respectively, except that they will never return NULL and instead loop
> forever trying to allocate memory.
> 
> If the first allocation attempt fails, a warning will be emitted,
> including a call trace.  Subsequent failures will suppress this warning.
> 
> These were added as helper functions for documentation and auditability.
> No future callers should be added.
  This looks like a cleaner approach than the previous one. You can add
Acked-by: Jan Kara <jack@suse.cz>
  for the JBD part.

								Honza
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  drivers/md/dm-region-hash.c |    2 +-
>  fs/btrfs/inode.c            |    2 +-
>  fs/gfs2/log.c               |    2 +-
>  fs/gfs2/rgrp.c              |   18 ++++---------
>  fs/jbd/transaction.c        |   11 ++------
>  fs/reiserfs/journal.c       |    3 +-
>  include/linux/slab.h        |   55 +++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 68 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
> --- a/drivers/md/dm-region-hash.c
> +++ b/drivers/md/dm-region-hash.c
> @@ -290,7 +290,7 @@ static struct dm_region *__rh_alloc(struct dm_region_hash *rh, region_t region)
>  
>  	nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC);
>  	if (unlikely(!nreg))
> -		nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL);
> +		nreg = kmalloc_nofail(sizeof(*nreg), GFP_NOIO);
>  
>  	nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
>  		      DM_RH_CLEAN : DM_RH_NOSYNC;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1967,7 +1967,7 @@ void btrfs_add_delayed_iput(struct inode *inode)
>  	if (atomic_add_unless(&inode->i_count, -1, 1))
>  		return;
>  
> -	delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);
> +	delayed = kmalloc_nofail(sizeof(*delayed), GFP_NOFS);
>  	delayed->inode = inode;
>  
>  	spin_lock(&fs_info->delayed_iput_lock);
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -709,7 +709,7 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl)
>  	}
>  	trace_gfs2_log_flush(sdp, 1);
>  
> -	ai = kzalloc(sizeof(struct gfs2_ail), GFP_NOFS | __GFP_NOFAIL);
> +	ai = kzalloc_nofail(sizeof(struct gfs2_ail), GFP_NOFS);
>  	INIT_LIST_HEAD(&ai->ai_ail1_list);
>  	INIT_LIST_HEAD(&ai->ai_ail2_list);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1440,8 +1440,8 @@ static struct gfs2_rgrpd *rgblk_free(struct gfs2_sbd *sdp, u64 bstart,
>  		rgrp_blk++;
>  
>  		if (!bi->bi_clone) {
> -			bi->bi_clone = kmalloc(bi->bi_bh->b_size,
> -					       GFP_NOFS | __GFP_NOFAIL);
> +			bi->bi_clone = kmalloc_nofail(bi->bi_bh->b_size,
> +					       GFP_NOFS);
>  			memcpy(bi->bi_clone + bi->bi_offset,
>  			       bi->bi_bh->b_data + bi->bi_offset,
>  			       bi->bi_len);
> @@ -1759,9 +1759,6 @@ fail:
>   * @block: the block
>   *
>   * Figure out what RG a block belongs to and add that RG to the list
> - *
> - * FIXME: Don't use NOFAIL
> - *
>   */
>  
>  void gfs2_rlist_add(struct gfs2_sbd *sdp, struct gfs2_rgrp_list *rlist,
> @@ -1789,8 +1786,8 @@ void gfs2_rlist_add(struct gfs2_sbd *sdp, struct gfs2_rgrp_list *rlist,
>  	if (rlist->rl_rgrps == rlist->rl_space) {
>  		new_space = rlist->rl_space + 10;
>  
> -		tmp = kcalloc(new_space, sizeof(struct gfs2_rgrpd *),
> -			      GFP_NOFS | __GFP_NOFAIL);
> +		tmp = kcalloc_nofail(new_space, sizeof(struct gfs2_rgrpd *),
> +			      GFP_NOFS);
>  
>  		if (rlist->rl_rgd) {
>  			memcpy(tmp, rlist->rl_rgd,
> @@ -1811,17 +1808,14 @@ void gfs2_rlist_add(struct gfs2_sbd *sdp, struct gfs2_rgrp_list *rlist,
>   * @rlist: the list of resource groups
>   * @state: the lock state to acquire the RG lock in
>   * @flags: the modifier flags for the holder structures
> - *
> - * FIXME: Don't use NOFAIL
> - *
>   */
>  
>  void gfs2_rlist_alloc(struct gfs2_rgrp_list *rlist, unsigned int state)
>  {
>  	unsigned int x;
>  
> -	rlist->rl_ghs = kcalloc(rlist->rl_rgrps, sizeof(struct gfs2_holder),
> -				GFP_NOFS | __GFP_NOFAIL);
> +	rlist->rl_ghs = kcalloc_nofail(rlist->rl_rgrps,
> +				sizeof(struct gfs2_holder), GFP_NOFS);
>  	for (x = 0; x < rlist->rl_rgrps; x++)
>  		gfs2_holder_init(rlist->rl_rgd[x]->rd_gl,
>  				state, 0,
> diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
> --- a/fs/jbd/transaction.c
> +++ b/fs/jbd/transaction.c
> @@ -98,14 +98,9 @@ static int start_this_handle(journal_t *journal, handle_t *handle)
>  	}
>  
>  alloc_transaction:
> -	if (!journal->j_running_transaction) {
> -		new_transaction = kzalloc(sizeof(*new_transaction),
> -						GFP_NOFS|__GFP_NOFAIL);
> -		if (!new_transaction) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -	}
> +	if (!journal->j_running_transaction)
> +		new_transaction = kzalloc_nofail(sizeof(*new_transaction),
> +						GFP_NOFS);
>  
>  	jbd_debug(3, "New handle %p going live.\n", handle);
>  
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2593,8 +2593,7 @@ static int journal_read(struct super_block *sb)
>  static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
>  {
>  	struct reiserfs_journal_list *jl;
> -	jl = kzalloc(sizeof(struct reiserfs_journal_list),
> -		     GFP_NOFS | __GFP_NOFAIL);
> +	jl = kzalloc_nofail(sizeof(struct reiserfs_journal_list), GFP_NOFS);
>  	INIT_LIST_HEAD(&jl->j_list);
>  	INIT_LIST_HEAD(&jl->j_working_list);
>  	INIT_LIST_HEAD(&jl->j_tail_bh_list);
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -334,6 +334,61 @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
>  	return kmalloc_node(size, flags | __GFP_ZERO, node);
>  }
>  
> +/**
> + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.
> + * @size: how many bytes of memory are required.
> + * @flags: the type of memory to allocate (see kmalloc).
> + *
> + * NOTE: no new callers of this function should be implemented!
> + * All memory allocations should be failable whenever possible.
> + */
> +static inline void *kmalloc_nofail(size_t size, gfp_t flags)
> +{
> +	void *ret;
> +
> +	for (;;) {
> +		ret = kmalloc(size, flags);
> +		if (ret)
> +			return ret;
> +		WARN_ONCE(1, "Out of memory, no fallback implemented "
> +				"(size=%lu flags=0x%x)\n",
> +				size, flags);
> +	}
> +}
> +
> +/**
> + * kcalloc_nofail - infinitely loop until kcalloc() succeeds.
> + * @n: number of elements.
> + * @size: element size.
> + * @flags: the type of memory to allocate (see kcalloc).
> + *
> + * NOTE: no new callers of this function should be implemented!
> + * All memory allocations should be failable whenever possible.
> + */
> +static inline void *kcalloc_nofail(size_t n, size_t size, gfp_t flags)
> +{
> +	void *ret;
> +
> +	for (;;) {
> +		ret = kcalloc(n, size, flags);
> +		if (ret)
> +			return ret;
> +		WARN_ONCE(1, "Out of memory, no fallback implemented "
> +				"(n=%lu size=%lu flags=0x%x)\n",
> +				n, size, flags);
> +	}
> +}
> +
> +/**
> + * kzalloc_nofail - infinitely loop until kzalloc() succeeds.
> + * @size: how many bytes of memory are required.
> + * @flags: the type of memory to allocate (see kzalloc).
> + */
> +static inline void *kzalloc_nofail(size_t size, gfp_t flags)
> +{
> +	return kmalloc_nofail(size, flags | __GFP_ZERO);
> +}
> +
>  void __init kmem_cache_init_late(void);
>  
>  #endif	/* _LINUX_SLAB_H */
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Cluster-devel] [patch 3/5] fs: add nofail variant of alloc_buffer_head

[Jan Kara]

On Tue 24-08-10 03:50:30, David Rientjes wrote:
> Add alloc_buffer_head_nofail().  This function is equivalent to
> alloc_buffer_head(), except that it will never return NULL and instead
> loop forever trying to allocate memory.
> 
> If the first allocation attempt fails, a warning will be emitted,
> including a call trace.  Subsequent failures will suppress this warning.
> 
> This was added as a helper function for documentation and auditability.
> No future callers should be added.
  Acked-by: Jan Kara <jack@suse.cz>
for the JBD part here as well.

								Honza
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  fs/buffer.c                 |   18 ++++++++++++++++++
>  fs/gfs2/log.c               |    2 +-
>  fs/jbd/journal.c            |    2 +-
>  include/linux/buffer_head.h |    1 +
>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -3238,6 +3238,24 @@ struct buffer_head *alloc_buffer_head(gfp_t gfp_flags)
>  }
>  EXPORT_SYMBOL(alloc_buffer_head);
>  
> +/*
> + * NOTE: no new callers of this function should be implemented!
> + * All memory allocations should be failable whenever possible.
> + */
> +struct buffer_head *alloc_buffer_head_nofail(gfp_t gfp_flags)
> +{
> +	struct buffer_head *ret;
> +
> +	for (;;) {
> +		ret = alloc_buffer_head(gfp_flags);
> +		if (ret)
> +			return ret;
> +		WARN_ONCE(1, "Out of memory; no fallback implemented "
> +				"(flags=0x%x)\n",
> +				gfp_flags);
> +	}
> +}
> +
>  void free_buffer_head(struct buffer_head *bh)
>  {
>  	BUG_ON(!list_empty(&bh->b_assoc_buffers));
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -523,7 +523,7 @@ struct buffer_head *gfs2_log_fake_buf(struct gfs2_sbd *sdp,
>  	u64 blkno = log_bmap(sdp, sdp->sd_log_flush_head);
>  	struct buffer_head *bh;
>  
> -	bh = alloc_buffer_head(GFP_NOFS | __GFP_NOFAIL);
> +	bh = alloc_buffer_head_nofail(GFP_NOFS);
>  	atomic_set(&bh->b_count, 1);
>  	bh->b_state = (1 << BH_Mapped) | (1 << BH_Uptodate) | (1 << BH_Lock);
>  	set_bh_page(bh, real->b_page, bh_offset(real));
> diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
> --- a/fs/jbd/journal.c
> +++ b/fs/jbd/journal.c
> @@ -301,7 +301,7 @@ int journal_write_metadata_buffer(transaction_t *transaction,
>  	 */
>  	J_ASSERT_BH(bh_in, buffer_jbddirty(bh_in));
>  
> -	new_bh = alloc_buffer_head(GFP_NOFS|__GFP_NOFAIL);
> +	new_bh = alloc_buffer_head_nofail(GFP_NOFS);
>  	/* keep subsequent assertions sane */
>  	new_bh->b_state = 0;
>  	init_buffer(new_bh, NULL, NULL);
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -176,6 +176,7 @@ void __breadahead(struct block_device *, sector_t block, unsigned int size);
>  struct buffer_head *__bread(struct block_device *, sector_t block, unsigned size);
>  void invalidate_bh_lrus(void);
>  struct buffer_head *alloc_buffer_head(gfp_t gfp_flags);
> +struct buffer_head *alloc_buffer_head_nofail(gfp_t gfp_flags);
>  void free_buffer_head(struct buffer_head * bh);
>  void unlock_buffer(struct buffer_head *bh);
>  void __lock_buffer(struct buffer_head *bh);
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Cluster-devel] [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

[Jan Kara]

On Thu 02-09-10 09:59:13, Jiri Slaby wrote:
> On 09/02/2010 03:02 AM, David Rientjes wrote:
> > --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -334,6 +334,57
> > @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
> > return kmalloc_node(size, flags | __GFP_ZERO, node); }
> >  
> > +/** + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.  +
> > * @size: how many bytes of memory are required.  + * @flags: the type
> > of memory to allocate (see kmalloc).  + * + * NOTE: no new callers of
> > this function should be implemented!  + * All memory allocations should
> > be failable whenever possible.  + */ +static inline void
> > *kmalloc_nofail(size_t size, gfp_t flags) +{ +	void *ret; + +	for
> > (;;) { +		ret = kmalloc(size, flags); +		if (ret) +
> > return ret; +		WARN_ON_ONCE(get_order(size) >
> > PAGE_ALLOC_COSTLY_ORDER);
> 
> This doesn't work as you expect. kmalloc will warn every time it fails.
> __GFP_NOFAIL used to disable the warning. Actually what's wrong with
> __GFP_NOFAIL? I cannot find a reason in the changelogs why the patches
> are needed.
  David should probably add the reasoning to the changelogs so that he
doesn't have to explain again and again ;). But if I understood it
correctly, the concern is that the looping checks slightly impact fast path
of the callers which do not need it. Generally, also looping for a long
time inside allocator isn't a nice thing but some callers aren't able to do
better for now to the patch is imperfect in this sence...

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

[Cluster-devel] [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

[Neil Brown]

On Thu, 2 Sep 2010 16:51:41 +0200
Jan Kara <jack@suse.cz> wrote:

> On Thu 02-09-10 09:59:13, Jiri Slaby wrote:
> > On 09/02/2010 03:02 AM, David Rientjes wrote:
> > > --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -334,6 +334,57
> > > @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
> > > return kmalloc_node(size, flags | __GFP_ZERO, node); }
> > >  
> > > +/** + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.  +
> > > * @size: how many bytes of memory are required.  + * @flags: the type
> > > of memory to allocate (see kmalloc).  + * + * NOTE: no new callers of
> > > this function should be implemented!  + * All memory allocations should
> > > be failable whenever possible.  + */ +static inline void
> > > *kmalloc_nofail(size_t size, gfp_t flags) +{ +	void *ret; + +	for
> > > (;;) { +		ret = kmalloc(size, flags); +		if (ret) +
> > > return ret; +		WARN_ON_ONCE(get_order(size) >
> > > PAGE_ALLOC_COSTLY_ORDER);
> > 
> > This doesn't work as you expect. kmalloc will warn every time it fails.
> > __GFP_NOFAIL used to disable the warning. Actually what's wrong with
> > __GFP_NOFAIL? I cannot find a reason in the changelogs why the patches
> > are needed.
>   David should probably add the reasoning to the changelogs so that he
> doesn't have to explain again and again ;). But if I understood it
> correctly, the concern is that the looping checks slightly impact fast path
> of the callers which do not need it. Generally, also looping for a long
> time inside allocator isn't a nice thing but some callers aren't able to do
> better for now to the patch is imperfect in this sence...
>

I'm actually a bit confused about this too.
I thought David said he was removing a branch on the *slow* path - which make
sense as you wouldn't even test NOFAIL until you had a failure.
Why are branches on the slow-path an issue??
This is an important question to me because I still hope to see the
swap-over-nfs patches merged eventually and they add a branch on the slow
path (if I remember correctly).

NeilBrown