On Thu, Apr 25, 2013 at 05:41:46PM -0500, Chandra Seetharaman wrote:
> In which case something along the lines of
>
> ---
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 3806088..3fb2fa6 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -203,7 +203,13 @@ xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t
> agno)
> if (pag) {
> ASSERT(atomic_read(&pag->pag_ref) >= 0);
> ref = atomic_inc_return(&pag->pag_ref);
> - }
> + } else
> + /*
> + * xfs_perag_get() is called with invalid agno,
> + * which cannot happen. This indicates a problem
> + * in the calling code.
> + */
> + BUG();
That's btrfs-style error handling. ;)
In reality, your patch is much worse than just letting the caller
dereference a null pointer, because it happens inside an RCU read
lock. That's guaranteed to hang the system, not just have the thread
that triggers a null pointer dereference go away. And further....
> rcu_read_unlock();
> trace_xfs_perag_get(mp, agno, ref, _RET_IP_);
... it means we can't catch the tracepoint with the bad agno in it.
> return pag;
> --------
>
> would be useful ?. Since we have a NULL pag, we will trip somewhere
> else. At least with this, there is a pointer to the debugger/sysadmin
> about where/what to look for (may be with more valuable/correct comment
> than above).
The current failure case is pretty damn obvious, so adding a BUG
doesn't make it any better. In fact, it makes it worse because it
prevents a caller from being able to handle a NULL perag return....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
