[PATCH] recover from iclog allocation failures

[PATCH] recover from iclog allocation failures

[Eric Sandeen]

A user in #xfs had some strange thing hogging up vmalloc
space, and after mounting several xfs filesystems with 
aggressive log memory usage, started hitting vmalloc failures 
which led to an oops.

I inserted a fake failure at i=3 in the iclog alloc loop, and 
this patch let me exit with a graceful "ENOMEM" instead of an 
oops.

Also, somehow the use of "uuid_mounted" has gone stale; after
the graceful mount failure, I got dup uuid errors on the next
mount.  This patch fixes that problem as well.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

---

Index: linux-2.6.24.noarch/fs/xfs/xfs_log.c
===================================================================
--- linux-2.6.24.noarch.orig/fs/xfs/xfs_log.c
+++ linux-2.6.24.noarch/fs/xfs/xfs_log.c
@@ -513,6 +513,8 @@ xfs_log_mount(xfs_mount_t	*mp,
 	}
 
 	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
+	if (!mp->m_log)
+		return ENOMEM;
 
 	/*
 	 * skip log recovery on a norecovery mount.  pretend it all
@@ -1219,6 +1221,13 @@ xlog_alloc_log(xfs_mount_t	*mp,
 		prev_iclog = iclog;
 
 		bp = xfs_buf_get_noaddr(log->l_iclog_size, mp->m_logdev_targp);
+		if (!iclog || !bp) {
+			if (iclog)
+				kmem_free(iclog, sizeof(xlog_in_core_t));
+			log->l_iclog_bufs = i;
+			xlog_dealloc_log(log);
+			return NULL;
+		}
 		if (!XFS_BUF_CPSEMA(bp))
 			ASSERT(0);
 		XFS_BUF_SET_IODONE_FUNC(bp, xlog_iodone);
Index: linux-2.6.24.noarch/fs/xfs/xfs_mount.c
===================================================================
--- linux-2.6.24.noarch.orig/fs/xfs/xfs_mount.c
+++ linux-2.6.24.noarch/fs/xfs/xfs_mount.c
@@ -1007,6 +1007,7 @@ xfs_mountfs(
 			error = XFS_ERROR(EINVAL);
 			goto error1;
 		}
+		uuid_mounted = 1;
 	}
 
 	/*

Re: [PATCH] recover from iclog allocation failures

[Christoph Hellwig]

On Fri, Feb 08, 2008 at 11:45:53PM -0600, Eric Sandeen wrote:
>  	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
> +	if (!mp->m_log)
> +		return ENOMEM;

Currently there's no allocations in there that should be able to fail.
But actually marking these KM_MAYFAIL would be a good idea.

> @@ -1219,6 +1221,13 @@ xlog_alloc_log(xfs_mount_t	*mp,
>  		prev_iclog = iclog;
>  
>  		bp = xfs_buf_get_noaddr(log->l_iclog_size, mp->m_logdev_targp);
> +		if (!iclog || !bp) {
> +			if (iclog)
> +				kmem_free(iclog, sizeof(xlog_in_core_t));
> +			log->l_iclog_bufs = i;
> +			xlog_dealloc_log(log);
> +			return NULL;
> +		}

Please check for iclog beeing NULL before trying to allocate the buffer,
and switch it to KM_MAYFAIL.  Given that there are two failing cases now
it would make sense to have goto-unwinding here.

Also once you touch the memory allocation feel free to remove the
useless casts of their return values.

> Index: linux-2.6.24.noarch/fs/xfs/xfs_mount.c
> ===================================================================
> --- linux-2.6.24.noarch.orig/fs/xfs/xfs_mount.c
> +++ linux-2.6.24.noarch/fs/xfs/xfs_mount.c
> @@ -1007,6 +1007,7 @@ xfs_mountfs(
>  			error = XFS_ERROR(EINVAL);
>  			goto error1;
>  		}
> +		uuid_mounted = 1;

How is this related to the rest of the patch?

Re: [PATCH] recover from iclog allocation failures

[Eric Sandeen]

Christoph Hellwig wrote:
> On Fri, Feb 08, 2008 at 11:45:53PM -0600, Eric Sandeen wrote:
>>  	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
>> +	if (!mp->m_log)
>> +		return ENOMEM;
> 
> Currently there's no allocations in there that should be able to fail.

Well, actually...

> But actually marking these KM_MAYFAIL would be a good idea.
> 
>> @@ -1219,6 +1221,13 @@ xlog_alloc_log(xfs_mount_t	*mp,
>>  		prev_iclog = iclog;
>>  
>>  		bp = xfs_buf_get_noaddr(log->l_iclog_size, mp->m_logdev_targp);

This was failing for him I think.

>> +		if (!iclog || !bp) {
>> +			if (iclog)
>> +				kmem_free(iclog, sizeof(xlog_in_core_t));
>> +			log->l_iclog_bufs = i;
>> +			xlog_dealloc_log(log);
>> +			return NULL;
>> +		}
> 
> Please check for iclog beeing NULL before trying to allocate the buffer,
> and switch it to KM_MAYFAIL.  Given that there are two failing cases now
> it would make sense to have goto-unwinding here.

hmm yeah probably so.

> Also once you touch the memory allocation feel free to remove the
> useless casts of their return values.
> 
>> Index: linux-2.6.24.noarch/fs/xfs/xfs_mount.c
>> ===================================================================
>> --- linux-2.6.24.noarch.orig/fs/xfs/xfs_mount.c
>> +++ linux-2.6.24.noarch/fs/xfs/xfs_mount.c
>> @@ -1007,6 +1007,7 @@ xfs_mountfs(
>>  			error = XFS_ERROR(EINVAL);
>>  			goto error1;
>>  		}
>> +		uuid_mounted = 1;
> 
> How is this related to the rest of the patch?

if we errored out, we returned from mount w/o taking the uuid back out
of the table.

-Eric

Re: [PATCH] recover from iclog allocation failures

[Eric Sandeen]

Eric Sandeen wrote:
> Christoph Hellwig wrote:

>>> Index: linux-2.6.24.noarch/fs/xfs/xfs_mount.c
>>> ===================================================================
>>> --- linux-2.6.24.noarch.orig/fs/xfs/xfs_mount.c
>>> +++ linux-2.6.24.noarch/fs/xfs/xfs_mount.c
>>> @@ -1007,6 +1007,7 @@ xfs_mountfs(
>>>  			error = XFS_ERROR(EINVAL);
>>>  			goto error1;
>>>  		}
>>> +		uuid_mounted = 1;
>> How is this related to the rest of the patch?
> 
> if we errored out, we returned from mount w/o taking the uuid back out
> of the table.

er, I must have messed up the tree, that's already there isn't it...

Let me respin this one... :)

-Eric