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

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

[Dave Chinner]

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 */
 
 
-- 
1.5.6.5

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

[Timothy Shimmin]

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

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

[Dave Chinner]

On Fri, Oct 31, 2008 at 03:33:49PM +1100, Timothy Shimmin wrote:
> 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.

Already had a local variable of the right type - not much
point in declaring a new variable to use as a list iterator
when you've already got a variable that is used as a list
iterator in another, non-overlapping part of the code ;)

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

The current trend is to unwind complex initialisation errors at
the place they occur, even if there is a destructor function for
a completely intialised object/subsystem. I just followed that
construct. And to be truly complete, it should also handle
trace buffer initialisation failure, which would make the
unwinding even more complex than it is above.

Given that this is a regression fix I didn't want to perturb the
log destructor code by making it have to handle partially set up
lists and objects....

If you still want me to push this into xlog_dealloc_log() I will,
just let me know.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Timothy Shimmin]

Dave Chinner wrote:
> On Fri, Oct 31, 2008 at 03:33:49PM +1100, Timothy Shimmin wrote:
>>> +
>>> +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.
> 
> Already had a local variable of the right type - not much
> point in declaring a new variable to use as a list iterator
> when you've already got a variable that is used as a list
> iterator in another, non-overlapping part of the code ;)
> 
Oh I see your point.
And if the variable was called iclog or something more generic
then it wouldn't be so bad.
However, in this case, the name is misleading for its use - it
infers the wrong object.
But this is irrelevant considering below :)

>> 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.
> 
> The current trend is to unwind complex initialisation errors at
> the place they occur, even if there is a destructor function for
> a completely intialised object/subsystem. I just followed that
> construct. And to be truly complete, it should also handle
> trace buffer initialisation failure, which would make the
> unwinding even more complex than it is above.
> 
> Given that this is a regression fix I didn't want to perturb the
> log destructor code by making it have to handle partially set up
> lists and objects....
> 
> If you still want me to push this into xlog_dealloc_log() I will,
> just let me know.
> 
:-)
Yeah, I'd prefer to reuse the same bit of code.
IMHO, it minimises the chance of getting it wrong by having it in one place etc.

Thanks muchly,
Tim.