Re: [PATCH] XFS: handle memory allocation failures during log initialisation

[Timothy Shimmin <tes@sgi.com>]

Hi Dave,

Dave Chinner wrote:
> When there is no memory left in the system, xfs_buf_get_noaddr()
> can fail. If this happens at mount time during xlog_alloc_log()
> we fail to catch the error and oops.
> 
> Catch the error from xfs_buf_get_noaddr(), and allow other memory
> allocations to fail and catch those errors too. Report the error
> to the console and fail the mount with ENOMEM.
> 
> Tested by manually injecting errors into xfs_buf_get_noaddr() and
> xlog_alloc_log().
> 
> Version 2:
> o remove unnecessary casts of the returned pointer from kmem_zalloc()
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_log.c |   39 ++++++++++++++++++++++++++++++++++++---
>  1 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5184017..92c20a8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -563,6 +563,11 @@ xfs_log_mount(
>  	}
>  
>  	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
> +	if (!mp->m_log) {
> +		cmn_err(CE_WARN, "XFS: Log allocation failed: No memory!");
> +		error = ENOMEM;
> +		goto out;
> +	}
>  
>  	/*
>  	 * Initialize the AIL now we have a log.
> @@ -601,6 +606,7 @@ xfs_log_mount(
>  	return 0;
>  error:
>  	xfs_log_unmount_dealloc(mp);
> +out:
>  	return error;
>  }	/* xfs_log_mount */
>  
> @@ -1217,7 +1223,9 @@ xlog_alloc_log(xfs_mount_t	*mp,
>  	int			i;
>  	int			iclogsize;
>  
> -	log = (xlog_t *)kmem_zalloc(sizeof(xlog_t), KM_SLEEP);
> +	log = kmem_zalloc(sizeof(xlog_t), KM_MAYFAIL);
> +	if (!log)
> +		return NULL;
>  
>  	log->l_mp	   = mp;
>  	log->l_targ	   = log_target;
> @@ -1249,6 +1257,8 @@ xlog_alloc_log(xfs_mount_t	*mp,
>  	xlog_get_iclog_buffer_size(mp, log);
>  
>  	bp = xfs_buf_get_empty(log->l_iclog_size, mp->m_logdev_targp);
> +	if (!bp)
> +		goto out_free_log;
>  	XFS_BUF_SET_IODONE_FUNC(bp, xlog_iodone);
>  	XFS_BUF_SET_BDSTRAT_FUNC(bp, xlog_bdstrat_cb);
>  	XFS_BUF_SET_FSPRIVATE2(bp, (unsigned long)1);
> @@ -1275,13 +1285,17 @@ xlog_alloc_log(xfs_mount_t	*mp,
>  	iclogsize = log->l_iclog_size;
>  	ASSERT(log->l_iclog_size >= 4096);
>  	for (i=0; i < log->l_iclog_bufs; i++) {
> -		*iclogp = (xlog_in_core_t *)
> -			  kmem_zalloc(sizeof(xlog_in_core_t), KM_SLEEP);
> +		*iclogp = kmem_zalloc(sizeof(xlog_in_core_t), KM_MAYFAIL);
> +		if (!*iclogp)
> +			goto out_free_iclog;
> +
>  		iclog = *iclogp;
>  		iclog->ic_prev = prev_iclog;
>  		prev_iclog = iclog;
>  
>  		bp = xfs_buf_get_noaddr(log->l_iclog_size, mp->m_logdev_targp);
> +		if (!bp)
> +			goto out_free_iclog;
>  		if (!XFS_BUF_CPSEMA(bp))
>  			ASSERT(0);
>  		XFS_BUF_SET_IODONE_FUNC(bp, xlog_iodone);
> @@ -1323,6 +1337,25 @@ xlog_alloc_log(xfs_mount_t	*mp,
>  	log->l_iclog->ic_prev = prev_iclog;	/* re-write 1st prev ptr */
>  
>  	return log;
> +
> +out_free_iclog:
> +	for (iclog = log->l_iclog; iclog; iclog = prev_iclog) {
> +		prev_iclog = iclog->ic_next;
> +		if (iclog->ic_bp) {
> +			sv_destroy(&iclog->ic_force_wait);
> +			sv_destroy(&iclog->ic_write_wait);
> +			xfs_buf_free(iclog->ic_bp);
> +			xlog_trace_iclog_dealloc(iclog);
> +		}
> +		kmem_free(iclog);
> +	}
> +	spinlock_destroy(&log->l_icloglock);
> +	spinlock_destroy(&log->l_grant_lock);
> +	xlog_trace_loggrant_dealloc(log);
> +	xfs_buf_free(log->l_xbuf);
> +out_free_log:
> +	kmem_free(log);
> +	return NULL;
>  }	/* xlog_alloc_log */
>  
>  

I would have done s/prev_iclog/next_iclog/
as I'm not sure why you look at it as previous.

However, I think it would be nicer to modify xlog_dealloc_log()
to handle less than l_iclog_bufs.
i.e put the code you have here into xlog_dealloc_log()
and do the deallocation in one place.

--Tim