xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] XFS: handle memory allocation failures during log initialisation
From: Timothy Shimmin <tes@xxxxxxx>
Date: Fri, 31 Oct 2008 15:33:49 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1225416366-3116-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1225416366-3116-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.17 (Macintosh/20080914)
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@xxxxxxxxxxxxx>
> ---
>  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

<Prev in Thread] Current Thread [Next in Thread>