XFS: handle memory allocation failures during log initialisation
PATCH 1/1 XFS: handle memory allocation failures during log initialisation
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()
--- fs/xfs/xfs_log.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 files changed, 36 insertions(+), 3 deletions(-)
fs/xfs/xfs_log.c +36 -3
@@ -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 */
Signature · 1 line
1.5.6.5
Hi Dave,
Quoted 99 lines · Dave Chinner
When there is no memory left in the system,
xfs_buf_get_noaddr()can fail. If this happens at mount time duringxlog_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()andxlog_alloc_log().Version 2:
o remove unnecessary casts of the returned pointer fromkmem_zalloc()Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_log.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 files changed, 36 insertions(+), 3 deletions(-)fs/xfs/xfs_log.c +29
-3@@ -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); + }
fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1361
Quoted 10 lines
fs/xfs/xfs_log.c +7
-0+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
Quoted 107 lines · Timothy Shimmin
Hi Dave,
Quoted 104 lines · Dave Chinner
When there is no memory left in the system,
xfs_buf_get_noaddr()can fail. If this happens at mount time duringxlog_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()andxlog_alloc_log().Version 2:
o remove unnecessary casts of the returned pointer fromkmem_zalloc()Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_log.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 files changed, 36 insertions(+), 3 deletions(-)fs/xfs/xfs_log.c +34
-3@@ -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:
Quoted 8 lines
fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1361
Quoted 5 lines
fs/xfs/xfs_log.c +2
-0+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 ;)
Quoted 4 lines
However, I think it would be nicer to modify
xlog_dealloc_log()to handle less thanl_iclog_bufs.
i.e put the code you have here intoxlog_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.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 26 lines · Dave Chinner
Quoted 16 lines · Timothy Shimmin
Quoted 16 lines
fs/xfs/xfs_log.c +16
-0+ +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);Quoted 8 lines
fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1359
Quoted 6 lines
fs/xfs/xfs_log.c +3
-0+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.
Quoted 5 lines
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 :)
Quoted 12 lines
Quoted 4 lines
However, I think it would be nicer to modify
xlog_dealloc_log()to handle less thanl_iclog_bufs.
i.e put the code you have here intoxlog_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.
Quoted 7 lines
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.