fixups for the changes in 4.13

fixups for the changes in 4.13

[Christoph Hellwig]

fix docs and assert locks for _ilocked helpers.  Rever the quota
change that does locking conditional on a flag, as that's never
the right way to do things.

[PATCH 1/4] xfs: fixup xfs_attr_get_ilocked

[Christoph Hellwig]

The comment mentioned the wrong lock.  Also add an ASSERT to assert
this locking precondition.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index ef8a1c75a467..de7b9bd30bec 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -114,12 +114,14 @@ xfs_inode_hasattr(
  * Overall external interface routines.
  *========================================================================*/
 
-/* Retrieve an extended attribute and its value.  Must have iolock. */
+/* Retrieve an extended attribute and its value.  Must have ilock. */
 int
 xfs_attr_get_ilocked(
 	struct xfs_inode	*ip,
 	struct xfs_da_args	*args)
 {
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+
 	if (!xfs_inode_hasattr(ip))
 		return -ENOATTR;
 	else if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL)
-- 
2.11.0

Re: [PATCH 1/4] xfs: fixup xfs_attr_get_ilocked

[Darrick J. Wong]

On Thu, Jul 13, 2017 at 01:33:01PM +0200, Christoph Hellwig wrote:
> The comment mentioned the wrong lock.  Also add an ASSERT to assert
> this locking precondition.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index ef8a1c75a467..de7b9bd30bec 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -114,12 +114,14 @@ xfs_inode_hasattr(
>   * Overall external interface routines.
>   *========================================================================*/
>  
> -/* Retrieve an extended attribute and its value.  Must have iolock. */
> +/* Retrieve an extended attribute and its value.  Must have ilock. */
>  int
>  xfs_attr_get_ilocked(
>  	struct xfs_inode	*ip,
>  	struct xfs_da_args	*args)
>  {
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> +
>  	if (!xfs_inode_hasattr(ip))
>  		return -ENOATTR;
>  	else if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL)
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] xfs: assert locking precondіtion in xfs_attr_list_int_ilocked

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_attr_list.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 545eca508d42..7740c8a5e736 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -463,6 +463,8 @@ xfs_attr_list_int_ilocked(
 {
 	struct xfs_inode		*dp = context->dp;
 
+	ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+
 	/*
 	 * Decide on what work routines to call based on the inode size.
 	 */
-- 
2.11.0

Re: [PATCH 2/4] xfs: assert locking precondіtion in xfs_attr_list_int_ilocked

[Darrick J. Wong]

On Thu, Jul 13, 2017 at 01:33:02PM +0200, Christoph Hellwig wrote:

Not sure about the garbled utf8 crud in the subject line, but otherwise
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_attr_list.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 545eca508d42..7740c8a5e736 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -463,6 +463,8 @@ xfs_attr_list_int_ilocked(
>  {
>  	struct xfs_inode		*dp = context->dp;
>  
> +	ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> +
>  	/*
>  	 * Decide on what work routines to call based on the inode size.
>  	 */
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] xfs: assert locking precondition in xfs_readlink_bmap_ilocked

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_symlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 12cd9cf7de41..23a50d7aa46a 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -61,6 +61,8 @@ xfs_readlink_bmap_ilocked(
 	int			fsblocks = 0;
 	int			offset;
 
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+
 	fsblocks = xfs_symlink_blocks(mp, pathlen);
 	error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
 	if (error)
-- 
2.11.0

Re: [PATCH 3/4] xfs: assert locking precondition in xfs_readlink_bmap_ilocked

[Darrick J. Wong]

On Thu, Jul 13, 2017 at 01:33:03PM +0200, Christoph Hellwig wrote:

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_symlink.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 12cd9cf7de41..23a50d7aa46a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -61,6 +61,8 @@ xfs_readlink_bmap_ilocked(
>  	int			fsblocks = 0;
>  	int			offset;
>  
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> +
>  	fsblocks = xfs_symlink_blocks(mp, pathlen);
>  	error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
>  	if (error)
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] Revert "xfs: grab dquots without taking the ilock"

[Christoph Hellwig]

This reverts commit 50e0bdbe9f48f98bb02eac7030d682f4716884ae.

The new XFS_QMOPT_NOLOCK isn't used at all, and conditional locking based
on a flag is always the wrong thing to do - we should be having helpers
that can be called without the lock instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_quota_defs.h |  2 --
 fs/xfs/xfs_dquot.c             | 14 ++++----------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index 2834574cb6e7..d69c772271cb 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -136,8 +136,6 @@ typedef uint16_t	xfs_qwarncnt_t;
  */
 #define XFS_QMOPT_INHERIT	0x1000000
 
-#define XFS_QMOPT_NOLOCK	0x2000000 /* don't ilock during dqget */
-
 /*
  * flags to xfs_trans_mod_dquot.
  */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index f89f7b5241e6..fd2ef8c2c9a7 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -472,23 +472,18 @@ xfs_qm_dqtobp(
 	struct xfs_mount	*mp = dqp->q_mount;
 	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
 	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
-	uint			lock_mode = 0;
+	uint			lock_mode;
 
 	quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags);
 	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
 
-	ASSERT(!(flags & XFS_QMOPT_NOLOCK) ||
-		xfs_isilocked(quotip, XFS_ILOCK_SHARED) ||
-		xfs_isilocked(quotip, XFS_ILOCK_EXCL));
-	if (!(flags & XFS_QMOPT_NOLOCK))
-		lock_mode = xfs_ilock_data_map_shared(quotip);
+	lock_mode = xfs_ilock_data_map_shared(quotip);
 	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
 		/*
 		 * Return if this type of quotas is turned off while we
 		 * didn't have the quota inode lock.
 		 */
-		if (lock_mode)
-			xfs_iunlock(quotip, lock_mode);
+		xfs_iunlock(quotip, lock_mode);
 		return -ESRCH;
 	}
 
@@ -498,8 +493,7 @@ xfs_qm_dqtobp(
 	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
 			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
 
-	if (lock_mode)
-		xfs_iunlock(quotip, lock_mode);
+	xfs_iunlock(quotip, lock_mode);
 	if (error)
 		return error;
 
-- 
2.11.0

Re: [PATCH 4/4] Revert "xfs: grab dquots without taking the ilock"

[Darrick J. Wong]

On Thu, Jul 13, 2017 at 01:33:04PM +0200, Christoph Hellwig wrote:
> This reverts commit 50e0bdbe9f48f98bb02eac7030d682f4716884ae.
> 
> The new XFS_QMOPT_NOLOCK isn't used at all,

It'll get used (eventually) by online fsck.

> and conditional locking based on a flag is always the wrong thing to
> do - we should be having helpers that can be called without the lock
> instead.

Fair enough.  How about this instead:

static int
__xfs_qm_dqget(mp, ip, id, type, flags, O_dqpp)
{
	/* existing xfs_qm_dqget implementation */
}

int
xfs_qm_dqget(mp, ip, id, type, flags, O_dqpp)
{
	if (flags & XFS_QMOPT_NOLOCK)
		return -EINVAL;
	return __xfs_qm_dqget(mp, ip, id, type, flags, O_dqpp);
}

/*
 * Grab the bare dquot for a given id, having locked the quota inode.
 * We don't support any caller QMOPT flags or other fancy behavior.
 */
int
xfs_qm_dqget_bare(mp, id, type, O_dqpp)
{
	ASSERT(xfs_isilocked(xfs_quota_inode(mp, type), XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
	return __xfs_qm_dqget(mp, NULL, id, type, XFS_QMOPT_NOLOCK, O_dqpp);
}

XFS_QMOPT_NOLOCK then becomes an internal flag (or I could make it into
an extra parameter to __xfs_qm_dqget) that can only be turned on
indirectly via the xfs_qm_dqget_bare helper.

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_quota_defs.h |  2 --
>  fs/xfs/xfs_dquot.c             | 14 ++++----------
>  2 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index 2834574cb6e7..d69c772271cb 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -136,8 +136,6 @@ typedef uint16_t	xfs_qwarncnt_t;
>   */
>  #define XFS_QMOPT_INHERIT	0x1000000
>  
> -#define XFS_QMOPT_NOLOCK	0x2000000 /* don't ilock during dqget */
> -
>  /*
>   * flags to xfs_trans_mod_dquot.
>   */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index f89f7b5241e6..fd2ef8c2c9a7 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -472,23 +472,18 @@ xfs_qm_dqtobp(
>  	struct xfs_mount	*mp = dqp->q_mount;
>  	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
>  	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
> -	uint			lock_mode = 0;
> +	uint			lock_mode;
>  
>  	quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags);
>  	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
>  
> -	ASSERT(!(flags & XFS_QMOPT_NOLOCK) ||
> -		xfs_isilocked(quotip, XFS_ILOCK_SHARED) ||
> -		xfs_isilocked(quotip, XFS_ILOCK_EXCL));
> -	if (!(flags & XFS_QMOPT_NOLOCK))
> -		lock_mode = xfs_ilock_data_map_shared(quotip);
> +	lock_mode = xfs_ilock_data_map_shared(quotip);
>  	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
>  		/*
>  		 * Return if this type of quotas is turned off while we
>  		 * didn't have the quota inode lock.
>  		 */
> -		if (lock_mode)
> -			xfs_iunlock(quotip, lock_mode);
> +		xfs_iunlock(quotip, lock_mode);
>  		return -ESRCH;
>  	}
>  
> @@ -498,8 +493,7 @@ xfs_qm_dqtobp(
>  	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
>  			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
>  
> -	if (lock_mode)
> -		xfs_iunlock(quotip, lock_mode);
> +	xfs_iunlock(quotip, lock_mode);
>  	if (error)
>  		return error;
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] Revert "xfs: grab dquots without taking the ilock"

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 09:31:27AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 13, 2017 at 01:33:04PM +0200, Christoph Hellwig wrote:
> > This reverts commit 50e0bdbe9f48f98bb02eac7030d682f4716884ae.
> > 
> > The new XFS_QMOPT_NOLOCK isn't used at all,
> 
> It'll get used (eventually) by online fsck.

As-is that will lead to buggy code. xfs_qm_dqread does transaction
allocations and disk reads, and none of those should be allowed
under the ILOCK ever.  That whole callchain is a mess and will need
some refactoring love to start with, e.g. including the dropping
of the ilock around the xfs_qm_dqread call in xfs_qm_dqget, and
a general mess of mixing up way to many things through the xfs_qm_dqget()
interface.

> > and conditional locking based on a flag is always the wrong thing to
> > do - we should be having helpers that can be called without the lock
> > instead.
> 
> Fair enough.  How about this instead:

Just as bad.

Re: [PATCH 4/4] Revert "xfs: grab dquots without taking the ilock"

[Darrick J. Wong]

On Thu, Jul 13, 2017 at 06:36:28PM +0200, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017 at 09:31:27AM -0700, Darrick J. Wong wrote:
> > On Thu, Jul 13, 2017 at 01:33:04PM +0200, Christoph Hellwig wrote:
> > > This reverts commit 50e0bdbe9f48f98bb02eac7030d682f4716884ae.
> > > 
> > > The new XFS_QMOPT_NOLOCK isn't used at all,
> > 
> > It'll get used (eventually) by online fsck.
> 
> As-is that will lead to buggy code. xfs_qm_dqread does transaction
> allocations and disk reads, and none of those should be allowed
> under the ILOCK ever.  That whole callchain is a mess and will need
> some refactoring love to start with, e.g. including the dropping
> of the ilock around the xfs_qm_dqread call in xfs_qm_dqget, and
> a general mess of mixing up way to many things through the xfs_qm_dqget()
> interface.

I thought we /could/ do disk reads with the ilock held -- we certainly
do that to read in bmap extents.  OTOH we never used to read dquot info
off the disk with the quota inode ilock held, so you're right, let's not
introduce that now.

The quota scrubber can be reworked to do all the things it needs to do
without taking any locks, so I'm ok with reverting this and leaving this
whole murky incohesive interface alone.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> > > and conditional locking based on a flag is always the wrong thing to
> > > do - we should be having helpers that can be called without the lock
> > > instead.
> > 
> > Fair enough.  How about this instead:
> 
> Just as bad.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html