diff mbox series

xfs: unlock inode when xfs_ioctl_setattr_get_trans can't get transaction

Message ID 20190422182541.GA4676@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: unlock inode when xfs_ioctl_setattr_get_trans can't get transaction | expand

Commit Message

Darrick J. Wong April 22, 2019, 6:25 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

We passed an inode into xfs_ioctl_setattr_get_trans with join_flags
indicating which locks are held on that inode.  If we can't allocate a
transaction then we need to unlock the inode before we bail out, like
all the other error paths do.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Chinner April 22, 2019, 11:20 p.m. UTC | #1 
On Mon, Apr 22, 2019 at 11:25:41AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We passed an inode into xfs_ioctl_setattr_get_trans with join_flags
> indicating which locks are held on that inode.  If we can't allocate a
> transaction then we need to unlock the inode before we bail out, like
> all the other error paths do.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Yup, that's a problem, and the function header comment says "we now
take full responsibility for unlocking the inode" so this looks
fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Darrick J. Wong April 22, 2019, 11:46 p.m. UTC | #2 
On Tue, Apr 23, 2019 at 09:20:25AM +1000, Dave Chinner wrote:
> On Mon, Apr 22, 2019 at 11:25:41AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We passed an inode into xfs_ioctl_setattr_get_trans with join_flags
> > indicating which locks are held on that inode.  If we can't allocate a
> > transaction then we need to unlock the inode before we bail out, like
> > all the other error paths do.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Yup, that's a problem, and the function header comment says "we now
> take full responsibility for unlocking the inode" so this looks
> fine.

Cool.  Thanks for the review!

--D

> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> -- 
> Dave Chinner
> david@fromorbit.com
Christoph Hellwig April 23, 2019, 6:19 a.m. UTC | #3 
On Mon, Apr 22, 2019 at 11:25:41AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We passed an inode into xfs_ioctl_setattr_get_trans with join_flags
> indicating which locks are held on that inode.  If we can't allocate a
> transaction then we need to unlock the inode before we bail out, like
> all the other error paths do.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 744a55092147..272ee6f037ff 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1173,7 +1173,7 @@  xfs_ioctl_setattr_get_trans(
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
 	if (error)
-		return ERR_PTR(error);
+		goto out_unlock;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL | join_flags);