Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690817 > unrolled thread

[PATCH 00/12] Assorted lustre fixes and improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/12] Assorted lustre fixes and improvements NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
    [PATCH 04/12] staging: lustre: ldlm: remove 'first_enq' arg from  ldlm_process_flock_lock() NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
    [PATCH 03/12] staging: lustre: llite: fix various issues with  ll_splice_alias. NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
      Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias. Oleg Drokin <oleg.drokin@intel.com> - 2017-07-19 05:00 +0200
        Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias. NeilBrown <neilb@suse.com> - 2017-07-19 06:40 +0200
          Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias. Oleg Drokin <oleg.drokin@intel.com> - 2017-07-19 08:20 +0200
    [PATCH 02/12] staging: lustre: llite: fix incorrect  DCACHE_DISCONNECTED test NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
    [PATCH 08/12] staging: lustre: ldlm: remove unused 'overlaps'  variable NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
    [PATCH 11/12] staging: lustre: ldlm: remove unnecessary 'ownlocks'  variable. NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200
    [PATCH 05/12] staging: lustre: ldlm: remove unused 'work_list' arg  from ldlm_process_flock_lock() NeilBrown <neilb@suse.com> - 2017-07-19 01:30 +0200

#1690817 — [PATCH 00/12] Assorted lustre fixes and improvements

Hi,
 I've been looking through lustre, finding things, and fixing some of
 them.
 There are three distinct subsets of patches here.
 - 1 patch to fix a few typos in comments
 - 2 patches to fix dcache issues, particularly around
    DCACHE_DISCONNECTED
 - 9 patches to clean up code in ldlm_flock().  These started
   as some simple list_entry tidy-ups, and just grew....

 The most interesting from a review perspective is
      staging: lustre: llite: fix various issues with ll_splice_alias.
 there are real locking issues in there, but its hard to be 100%
 sure I've fixed them all and not introduced new issues.
 It would be *really* nice if we could get rid of the concept of
 "invalid" dentries, but I don't yet understand it well enough to
 propose and alternative.

Thanks,
NeilBrown

---

NeilBrown (12):
      staging: lustre: fix minor typos in comments
      staging: lustre: llite: fix incorrect DCACHE_DISCONNECTED test
      staging: lustre: llite: fix various issues with ll_splice_alias.
      staging: lustre: ldlm: remove 'first_enq' arg from ldlm_process_flock_lock()
      staging: lustre: ldlm: remove unused 'work_list' arg from ldlm_process_flock_lock()
      staging: lustre: ldlm: remove unneeded 'err' arg to ldlm_process_flock_lock()
      staging: lustre: ldlm: remove 'flags' arg from ldlm_process_flock_lock()
      staging: lustre: ldlm: remove unused 'overlaps' variable
      staging: lustre: ldlm: remove 'flags' arg from ldlm_flock_destroy()
      staging: lustre: ldlm: tidy list walking in ldlm_flock()
      staging: lustre: ldlm: remove unnecessary 'ownlocks' variable.
      staging: lustre: ldlm: remove unused field 'fwd_generation'


 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c    |  208 ++++----------------
 .../staging/lustre/lustre/llite/llite_internal.h   |    2 
 drivers/staging/lustre/lustre/llite/namei.c        |   71 +++----
 drivers/staging/lustre/lustre/mdc/mdc_locks.c      |    4 
 4 files changed, 80 insertions(+), 205 deletions(-)

--
Signature

[toc] | [next] | [standalone]

#1690818 — [PATCH 04/12] staging: lustre: ldlm: remove 'first_enq' arg from ldlm_process_flock_lock()

it is only ever set to '1', so we can just assume that and remove the code.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
index b7f28b39c7b3..8ba3eaf49c65 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
@@ -121,15 +121,9 @@ ldlm_flock_destroy(struct ldlm_lock *lock, enum ldlm_mode mode, __u64 flags)
  * It is also responsible for splitting a lock if a portion of the lock
  * is released.
  *
- * If \a first_enq is 0 (ie, called from ldlm_reprocess_queue):
- *   - blocking ASTs have already been sent
- *
- * If \a first_enq is 1 (ie, called from ldlm_lock_enqueue):
- *   - blocking ASTs have not been sent yet, so list of conflicting locks
- *     would be collected and ASTs sent.
  */
 static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
-				   int first_enq, enum ldlm_error *err,
+				   enum ldlm_error *err,
 				   struct list_head *work_list)
 {
 	struct ldlm_resource *res = req->l_resource;
@@ -197,11 +191,6 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
 			if (!ldlm_flocks_overlap(lock, req))
 				continue;
 
-			if (!first_enq) {
-				reprocess_failed = 1;
-				continue;
-			}
-
 			if (*flags & LDLM_FL_BLOCK_NOWAIT) {
 				ldlm_flock_destroy(req, mode, *flags);
 				*err = -EAGAIN;
@@ -605,7 +594,7 @@ ldlm_flock_completion_ast(struct ldlm_lock *lock, __u64 flags, void *data)
 		/* We need to reprocess the lock to do merges or splits
 		 * with existing locks owned by this process.
 		 */
-		ldlm_process_flock_lock(lock, &noreproc, 1, &err, NULL);
+		ldlm_process_flock_lock(lock, &noreproc, &err, NULL);
 	}
 	unlock_res_and_lock(lock);
 	return rc;

[toc] | [prev] | [next] | [standalone]

#1690819 — [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.

1/ The testing of DCACHE_DISCONNECTED is wrong.
  see upstream commit da093a9b76ef ("dcache: d_splice_alias should
  ignore DCACHE_DISCONNECTED")

  As this is a notoriously difficult piece of code to get right,
  it makes sense to use d_splice_alias() directly and no try to
  create a local version of it.

2/ ll_find_alias() currently:
     locks and alias
     checks that it is the one we want
     unlock it
     locks it again
     gets a reference
     unlocks it

   This isn't safe.  Anything could happen to the dentry while we
   don't hold a reference.  We need to dget the reference while
   still holding the lock.

3/ The d_move() in ll_splice_alias() is pointless.  We have
    already checked the hash, name, and parent are the same, and
    these are the only fields that d_move() will change.

4/ The call to d_add() is outside of any locking. This makes it
   possible for two identical dentries to be added to the same
   inode, which would cause confusion.

   Prior to 4.7, i_mutex would have provided exclusion, but since
   the VFS supports parallel lookups, only a shared lock is held
   on i_mutex.

   Because ll_d_init() creates a dentry in a state where
   ll_dcompare will no recognize it, the VFS provides no guarantee
   that we won't have two concurrent calls to ll_lookup_dn() for
   the same parent/name.


So: rename ll_find_alias() to ll_find_invalid_alias() and have it
just focus on finding an invalid alias.

For directories, we can just use d__splice_alias() directly.
There must only be one alias for a directory, and
ll_splice_alias() will find it where it is "invalid" or not.

For non-directories, we call ll_find_invalid_alias(), and either
use the result or call d_add().  We need a lock to protect from
races with other threads calling ll_find_invalid_alias() and
d_add() at the same time. lli_lock seems suitable for this
purpose.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/llite/namei.c |   69 +++++++++++++--------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 293a3180ec70..6204c3e70d45 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -378,75 +378,74 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2)
 }
 
 /*
- * try to reuse three types of dentry:
- * 1. unhashed alias, this one is unhashed by d_invalidate (but it may be valid
- *    by concurrent .revalidate).
- * 2. INVALID alias (common case for no valid ldlm lock held, but this flag may
- *    be cleared by others calling d_lustre_revalidate).
- * 3. DISCONNECTED alias.
+ * Try to find an "invalid" alias.  i.e. one that was unhashed by
+ * d_invalidate(), or that was instantiated with no valid ldlm lock.
+ * These can be rehased by d_lustre_revalidate(), which could race
+ * with this code.
  */
-static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
+static struct dentry *ll_find_invalid_alias(struct inode *inode,
+					    struct dentry *dentry)
 {
-	struct dentry *alias, *discon_alias, *invalid_alias;
+	struct dentry *alias, *invalid_alias = NULL;
 
 	if (hlist_empty(&inode->i_dentry))
 		return NULL;
 
-	discon_alias = NULL;
-	invalid_alias = NULL;
-
 	spin_lock(&inode->i_lock);
 	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
 		LASSERT(alias != dentry);
 
 		spin_lock(&alias->d_lock);
-		if ((alias->d_flags & DCACHE_DISCONNECTED) &&
-		    S_ISDIR(inode->i_mode))
-			/* LASSERT(last_discon == NULL); LU-405, bz 20055 */
-			discon_alias = alias;
-		else if (alias->d_parent == dentry->d_parent	     &&
-			 alias->d_name.hash == dentry->d_name.hash       &&
-			 alias->d_name.len == dentry->d_name.len	 &&
-			 memcmp(alias->d_name.name, dentry->d_name.name,
-				dentry->d_name.len) == 0)
+		if (alias->d_parent == dentry->d_parent       &&
+		    alias->d_name.hash == dentry->d_name.hash &&
+		    alias->d_name.len == dentry->d_name.len   &&
+		    memcmp(alias->d_name.name, dentry->d_name.name,
+			   dentry->d_name.len) == 0) {
+			dget_dlock(alias);
 			invalid_alias = alias;
+		}
 		spin_unlock(&alias->d_lock);
 
 		if (invalid_alias)
 			break;
 	}
-	alias = invalid_alias ?: discon_alias ?: NULL;
-	if (alias) {
-		spin_lock(&alias->d_lock);
-		dget_dlock(alias);
-		spin_unlock(&alias->d_lock);
-	}
 	spin_unlock(&inode->i_lock);
 
-	return alias;
+	return invalid_alias;
 }
 
 /*
- * Similar to d_splice_alias(), but lustre treats invalid alias
- * similar to DCACHE_DISCONNECTED, and tries to use it anyway.
+ * Similar to d_splice_alias(), but also look for an "invalid" alias,
+ * specific to lustre, and use that if found.
  */
 struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
 {
-	if (inode) {
-		struct dentry *new = ll_find_alias(inode, de);
+	if (inode && !S_ISDIR(inode->i_mode)) {
+		struct ll_inode_info *lli = ll_i2info(inode);
+		struct dentry *new;
+
+		/* We need lli_lock here as another thread could
+		 * be running this code, and i_lock cannot protect us.
+		 */
+		spin_lock(&lli->lli_lock);
+		new = ll_find_invalid_alias(inode, de);
+		if (!new)
+			d_add(de, inode);
+		spin_lock(&lli->lli_lock);
 
 		if (new) {
-			d_move(new, de);
 			iput(inode);
 			CDEBUG(D_DENTRY,
 			       "Reuse dentry %p inode %p refc %d flags %#x\n",
 			      new, d_inode(new), d_count(new), new->d_flags);
 			return new;
 		}
+		return de;
 	}
-	d_add(de, inode);
-	CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
-	       de, d_inode(de), d_count(de), de->d_flags);
+	de = d_splice_alias(inode, de);
+	if (!IS_ERR(de))
+		CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
+		       de, d_inode(de), d_count(de), de->d_flags);
 	return de;
 }
 

[toc] | [prev] | [next] | [standalone]

#1690926 — Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.

Unfortunately this patch causes insta-crash on first stat call after mount.
Sorry, I cannot dig into this deeper right this moment, but I will a bit later.
I am adding Al that we discussed this code at some length and he found no problems
here, so I am a bit surprised by your findings.
Also the reason we reinvent the d_splice_alias is because we need to
splice not just directories, but also regular files.

I also am less sure by your previous DCACHE_DISCONECTED patch that we in fact might
still need, I just need to dig up a test case for that.

Thanks for looking into it!

[  170.000858] Lustre: Mounted lustre-client
[  172.799813] Lustre: DEBUG MARKER: Using TIMEOUT=20
[  186.627954] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8
[  186.628742] IP: __lock_acquire+0x125/0x1370
[  186.629137] PGD 0 
[  186.629138] P4D 0 
[  186.629496] 
[  186.630216] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[  186.630613] Modules linked in: osc(C) mgc(C) lustre(C) lmv(C) fld(C) mdc(C) fid(C) lov(C) ksocklnd(C) ptlrpc(C) obdclass(C) lnet(C) sha512_ssse3 sha512_generic crc32_generic libcfs(C) joydev pcspkr i2c_piix4 virtio_console rpcsec_gss_krb5 ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm serio_raw virtio_blk floppy
[  186.633238] CPU: 2 PID: 10897 Comm: sh Tainted: G         C      4.13.0-rc1-vm-nfs+ #154
[  186.633990] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  186.634402] task: ffff8800d6a1c180 task.stack: ffffc900066a0000
[  186.634807] RIP: 0010:__lock_acquire+0x125/0x1370
[  186.635206] RSP: 0018:ffffc900066a3750 EFLAGS: 00010002
[  186.635597] RAX: 0000000000000046 RBX: 0000000000000001 RCX: 0000000000000000
[  186.636013] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000000
[  186.636432] RBP: ffffc900066a3810 R08: ffffffffa05221a9 R09: 0000000000000000
[  186.636844] R10: 0000000000000000 R11: ffff8800d6a1c180 R12: 0000000000000001
[  186.637264] R13: 0000000000000000 R14: 0000000000000001 R15: 00000000000000a8
[  186.637679] FS:  00007fd679eaa700(0000) GS:ffff88011a200000(0000) knlGS:0000000000000000
[  186.638402] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  186.638803] CR2: 00000000000000a8 CR3: 0000000109c0b000 CR4: 00000000000006e0
[  186.639228] Call Trace:
[  186.639589]  lock_acquire+0xe3/0x1d0
[  186.639962]  ? lock_acquire+0xe3/0x1d0
[  186.640352]  ? ll_lookup_it_finish+0x379/0xca0 [lustre]
[  186.640747]  _raw_spin_lock+0x34/0x70
[  186.641130]  ? ll_lookup_it_finish+0x379/0xca0 [lustre]
[  186.641529]  ll_lookup_it_finish+0x379/0xca0 [lustre]
[  186.641943]  ? req_capsule_server_get+0x15/0x20 [ptlrpc]
[  186.642368]  ? lmv_revalidate_slaves+0x790/0x790 [lmv]
[  186.642779]  ll_lookup_it+0x26d/0x820 [lustre]
[  186.643175]  ll_lookup_nd+0x162/0x1a0 [lustre]
[  186.643575]  lookup_slow+0x132/0x220
[  186.643947]  ? __wake_up+0x23/0x50
[  186.644322]  walk_component+0x1bf/0x350
[  186.644714]  link_path_walk+0x1b8/0x630
[  186.645097]  path_lookupat+0x99/0x220
[  186.645459]  ? __kernel_map_pages+0x131/0x140
[  186.645830]  ? __kernel_map_pages+0x131/0x140
[  186.646206]  filename_lookup+0xb8/0x1a0
[  186.646575]  ? __check_object_size+0xb1/0x1a0
[  186.646952]  ? strncpy_from_user+0x4d/0x160
[  186.647326]  user_path_at_empty+0x36/0x40
[  186.647694]  ? user_path_at_empty+0x36/0x40
[  186.648068]  vfs_statx+0x76/0xe0
[  186.648429]  SYSC_newstat+0x3d/0x70
[  186.648789]  ? trace_hardirqs_on_caller+0xf4/0x190
[  186.649171]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[  186.649547]  SyS_newstat+0xe/0x10
[  186.649906]  entry_SYSCALL_64_fastpath+0x1f/0xbe
[  186.650289] RIP: 0033:0x7fd679599475
[  186.650651] RSP: 002b:00007ffc2e568fc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000004
[  186.651350] RAX: ffffffffffffffda RBX: 00007fd679862ae0 RCX: 00007fd679599475
[  186.651758] RDX: 00007ffc2e568fe0 RSI: 00007ffc2e568fe0 RDI: 000000eec2877730
[  186.652171] RBP: 00007fd679862ae0 R08: 000000eec2cafcb0 R09: 0000000000000008
[  186.652579] R10: 000000eec2cafcb0 R11: 0000000000000246 R12: 0000000000000020
[  186.652982] R13: 000000eec2cc7e10 R14: 000000eec2cb2190 R15: 00007ffc2e5690f8
[  186.653393] Code: c8 65 48 33 3c 25 28 00 00 00 44 89 e0 0f 85 91 0d 00 00 48 81 c4 90 00 00 00 5b 41 5a 41 5c 41 5d 41 5e 41 5f 5d 49 8d 62 f8 c3 <49> 81 3f 00 90 6f 82 41 bc 00 00 00 00 44 0f 45 e2 83 fe 01 0f 
[  186.654536] RIP: __lock_acquire+0x125/0x1370 RSP: ffffc900066a3750
[  186.654933] CR2: 00000000000000a8


On Jul 18, 2017, at 7:26 PM, NeilBrown wrote:

> 1/ The testing of DCACHE_DISCONNECTED is wrong.
>  see upstream commit da093a9b76ef ("dcache: d_splice_alias should
>  ignore DCACHE_DISCONNECTED")
> 
>  As this is a notoriously difficult piece of code to get right,
>  it makes sense to use d_splice_alias() directly and no try to
>  create a local version of it.
> 
> 2/ ll_find_alias() currently:
>     locks and alias
>     checks that it is the one we want
>     unlock it
>     locks it again
>     gets a reference
>     unlocks it
> 
>   This isn't safe.  Anything could happen to the dentry while we
>   don't hold a reference.  We need to dget the reference while
>   still holding the lock.
> 
> 3/ The d_move() in ll_splice_alias() is pointless.  We have
>    already checked the hash, name, and parent are the same, and
>    these are the only fields that d_move() will change.
> 
> 4/ The call to d_add() is outside of any locking. This makes it
>   possible for two identical dentries to be added to the same
>   inode, which would cause confusion.
> 
>   Prior to 4.7, i_mutex would have provided exclusion, but since
>   the VFS supports parallel lookups, only a shared lock is held
>   on i_mutex.
> 
>   Because ll_d_init() creates a dentry in a state where
>   ll_dcompare will no recognize it, the VFS provides no guarantee
>   that we won't have two concurrent calls to ll_lookup_dn() for
>   the same parent/name.
> 
> 
> So: rename ll_find_alias() to ll_find_invalid_alias() and have it
> just focus on finding an invalid alias.
> 
> For directories, we can just use d__splice_alias() directly.
> There must only be one alias for a directory, and
> ll_splice_alias() will find it where it is "invalid" or not.
> 
> For non-directories, we call ll_find_invalid_alias(), and either
> use the result or call d_add().  We need a lock to protect from
> races with other threads calling ll_find_invalid_alias() and
> d_add() at the same time. lli_lock seems suitable for this
> purpose.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> drivers/staging/lustre/lustre/llite/namei.c |   69 +++++++++++++--------------
> 1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
> index 293a3180ec70..6204c3e70d45 100644
> --- a/drivers/staging/lustre/lustre/llite/namei.c
> +++ b/drivers/staging/lustre/lustre/llite/namei.c
> @@ -378,75 +378,74 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2)
> }
> 
> /*
> - * try to reuse three types of dentry:
> - * 1. unhashed alias, this one is unhashed by d_invalidate (but it may be valid
> - *    by concurrent .revalidate).
> - * 2. INVALID alias (common case for no valid ldlm lock held, but this flag may
> - *    be cleared by others calling d_lustre_revalidate).
> - * 3. DISCONNECTED alias.
> + * Try to find an "invalid" alias.  i.e. one that was unhashed by
> + * d_invalidate(), or that was instantiated with no valid ldlm lock.
> + * These can be rehased by d_lustre_revalidate(), which could race
> + * with this code.
>  */
> -static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
> +static struct dentry *ll_find_invalid_alias(struct inode *inode,
> +					    struct dentry *dentry)
> {
> -	struct dentry *alias, *discon_alias, *invalid_alias;
> +	struct dentry *alias, *invalid_alias = NULL;
> 
> 	if (hlist_empty(&inode->i_dentry))
> 		return NULL;
> 
> -	discon_alias = NULL;
> -	invalid_alias = NULL;
> -
> 	spin_lock(&inode->i_lock);
> 	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> 		LASSERT(alias != dentry);
> 
> 		spin_lock(&alias->d_lock);
> -		if ((alias->d_flags & DCACHE_DISCONNECTED) &&
> -		    S_ISDIR(inode->i_mode))
> -			/* LASSERT(last_discon == NULL); LU-405, bz 20055 */
> -			discon_alias = alias;
> -		else if (alias->d_parent == dentry->d_parent	     &&
> -			 alias->d_name.hash == dentry->d_name.hash       &&
> -			 alias->d_name.len == dentry->d_name.len	 &&
> -			 memcmp(alias->d_name.name, dentry->d_name.name,
> -				dentry->d_name.len) == 0)
> +		if (alias->d_parent == dentry->d_parent       &&
> +		    alias->d_name.hash == dentry->d_name.hash &&
> +		    alias->d_name.len == dentry->d_name.len   &&
> +		    memcmp(alias->d_name.name, dentry->d_name.name,
> +			   dentry->d_name.len) == 0) {
> +			dget_dlock(alias);
> 			invalid_alias = alias;
> +		}
> 		spin_unlock(&alias->d_lock);
> 
> 		if (invalid_alias)
> 			break;
> 	}
> -	alias = invalid_alias ?: discon_alias ?: NULL;
> -	if (alias) {
> -		spin_lock(&alias->d_lock);
> -		dget_dlock(alias);
> -		spin_unlock(&alias->d_lock);
> -	}
> 	spin_unlock(&inode->i_lock);
> 
> -	return alias;
> +	return invalid_alias;
> }
> 
> /*
> - * Similar to d_splice_alias(), but lustre treats invalid alias
> - * similar to DCACHE_DISCONNECTED, and tries to use it anyway.
> + * Similar to d_splice_alias(), but also look for an "invalid" alias,
> + * specific to lustre, and use that if found.
>  */
> struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
> {
> -	if (inode) {
> -		struct dentry *new = ll_find_alias(inode, de);
> +	if (inode && !S_ISDIR(inode->i_mode)) {
> +		struct ll_inode_info *lli = ll_i2info(inode);
> +		struct dentry *new;
> +
> +		/* We need lli_lock here as another thread could
> +		 * be running this code, and i_lock cannot protect us.
> +		 */
> +		spin_lock(&lli->lli_lock);
> +		new = ll_find_invalid_alias(inode, de);
> +		if (!new)
> +			d_add(de, inode);
> +		spin_lock(&lli->lli_lock);
> 
> 		if (new) {
> -			d_move(new, de);
> 			iput(inode);
> 			CDEBUG(D_DENTRY,
> 			       "Reuse dentry %p inode %p refc %d flags %#x\n",
> 			      new, d_inode(new), d_count(new), new->d_flags);
> 			return new;
> 		}
> +		return de;
> 	}
> -	d_add(de, inode);
> -	CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
> -	       de, d_inode(de), d_count(de), de->d_flags);
> +	de = d_splice_alias(inode, de);
> +	if (!IS_ERR(de))
> +		CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
> +		       de, d_inode(de), d_count(de), de->d_flags);
> 	return de;
> }
> 
> 

[toc] | [prev] | [next] | [standalone]

#1690959 — Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 18 2017, Oleg Drokin wrote:

> Unfortunately this patch causes insta-crash on first stat call after mount.
> Sorry, I cannot dig into this deeper right this moment, but I will a bit later.

V.strange.  The crash suggests that the lock, and hence the inode, is
not initialized.  I cannot see how that might happen.
though...

>> +		spin_lock(&lli->lli_lock);
>> +		new = ll_find_invalid_alias(inode, de);
>> +		if (!new)
>> +			d_add(de, inode);
>> +		spin_lock(&lli->lli_lock);

Had it not crashed, it would have deadlocked.  That second spin_lock()
should be spin_unlock() :-( I don't *think* that would have caused this crash...

> I am adding Al that we discussed this code at some length and he found no problems
> here, so I am a bit surprised by your findings.

I'd be very happy to read Al's thoughts.

> Also the reason we reinvent the d_splice_alias is because we need to
> splice not just directories, but also regular files.

I see that.  A key simplification I bring is that directories and
non-directories can be handled separately.  d_splice_alias() does
all we need for directories, and nothing useful for non-dirs.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1691006 — Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.

On Jul 19, 2017, at 12:33 AM, NeilBrown wrote:

> On Tue, Jul 18 2017, Oleg Drokin wrote:
> 
>> Unfortunately this patch causes insta-crash on first stat call after mount.
>> Sorry, I cannot dig into this deeper right this moment, but I will a bit later.
> 
> V.strange.  The crash suggests that the lock, and hence the inode, is
> not initialized.  I cannot see how that might happen.
> though...
> 
>>> +		spin_lock(&lli->lli_lock);
>>> +		new = ll_find_invalid_alias(inode, de);
>>> +		if (!new)
>>> +			d_add(de, inode);
>>> +		spin_lock(&lli->lli_lock);
> 
> Had it not crashed, it would have deadlocked.  That second spin_lock()
> should be spin_unlock() :-( I don't *think* that would have caused this crash…

No, that's not it.
-	d_add(de, inode);
-	CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
-	       de, d_inode(de), d_count(de), de->d_flags);
+	de = d_splice_alias(inode, de);
+	if (!IS_ERR(de))
+		CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
+		       de, d_inode(de), d_count(de), de->d_flags);
	return de;

This is likely it.
d_splice_alias would return NULL if there's no alias, after d_add of the de.
But ll_splice_alias callers expect either an ERR_PTR on error or dentry to use
otherwise.
So in ll_lookup_it_finish() we have:
        alias = ll_splice_alias(inode, *de);
        if (IS_ERR(alias)) {
                rc = PTR_ERR(alias);
                goto out;
        }
        *de = alias;
…
                if (md_revalidate_lock(ll_i2mdexp(parent), &parent_it, &fid,
                                       NULL)) {
===>>> whoops!          d_lustre_revalidate(*de);
                        ll_intent_release(&parent_it);
                }

>> I am adding Al that we discussed this code at some length and he found no problems
>> here, so I am a bit surprised by your findings.
> I'd be very happy to read Al's thoughts.

Some of the discussions were in lkml under subject of "More parallel
atomic_open/d_splice_alias fun with NFS and possibly more FSes"
in July 2016, in between nfs stuff.
There was more but I cannot readily find it.

>> Also the reason we reinvent the d_splice_alias is because we need to
>> splice not just directories, but also regular files.
> 
> I see that.  A key simplification I bring is that directories and
> non-directories can be handled separately.  d_splice_alias() does
> all we need for directories, and nothing useful for non-dirs.

I see.
I still need to  think some more about this whole thing.

Please see commit 99f1c013194e64d4b67d5d318148303b0e1585e1 for double d_add
of the same dentry fix.6

[toc] | [prev] | [next] | [standalone]

#1690820 — [PATCH 02/12] staging: lustre: llite: fix incorrect DCACHE_DISCONNECTED test

It is almost always wrong to test DCACHE_DISCONNECTED, except in
"exportfs" code.
The flag tells us that this dentry *might* not be connected to the
root through a chain of d_parent links.  Following the d_parent
to an IS_ROOT() dentry *might* find one that is on the s_anon list
rather than s_root.

The code here need to know if it is safe to call __d_drop(), and
the correct test is "!IS_ROOT(dentry)".
If a dentry IS_ROOT(), then it might be the filesystem root, or it
might be the root of a DCACHE_DISCONNECTED tree, and so be on
the s_anon list.  In these two cases it should not be __d_drop()ed.
If !IS_ROOT(), then the dentry is attached to its parent through
d_subdir, and can safely be unhashed.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 .../staging/lustre/lustre/llite/llite_internal.h   |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
index cd3311abf999..4854985bf4d3 100644
--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
@@ -1299,7 +1299,7 @@ static inline void d_lustre_invalidate(struct dentry *dentry, int nested)
 	 * If we unhashed such a dentry, unmount would not be able to find
 	 * it and busy inodes would be reported.
 	 */
-	if (d_count(dentry) == 0 && !(dentry->d_flags & DCACHE_DISCONNECTED))
+	if (d_count(dentry) == 0 && !IS_ROOT(dentry))
 		__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
 }

[toc] | [prev] | [next] | [standalone]

#1690823 — [PATCH 08/12] staging: lustre: ldlm: remove unused 'overlaps' variable

'overlaps' is never used, only incremented.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
index b6d5a83e61cd..0fb2c882ab4a 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
@@ -133,7 +133,6 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
 	struct ldlm_lock *new2 = NULL;
 	enum ldlm_mode mode = req->l_req_mode;
 	int added = (mode == LCK_NL);
-	int overlaps = 0;
 	int splitted = 0;
 	const struct ldlm_callback_suite null_cbs = { };
 
@@ -226,8 +225,6 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
 		    lock->l_policy_data.l_flock.start)
 			break;
 
-		++overlaps;
-
 		if (new->l_policy_data.l_flock.start <=
 		    lock->l_policy_data.l_flock.start) {
 			if (new->l_policy_data.l_flock.end <

[toc] | [prev] | [next] | [standalone]

#1690825 — [PATCH 11/12] staging: lustre: ldlm: remove unnecessary 'ownlocks' variable.

Now that the code has been simplified, 'ownlocks' is not
necessary.

The loop which sets it exits with 'lock' having the same value as
'ownlocks', or point to the head of the list if ownlocks is NULL.

The current code then tests ownlocks and sets 'lock' to exact the
value that it currently has.

So discard 'ownlocks'.

Also remove unnecessary initialization of 'lock'.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
index 58227728a002..4e8808103437 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
@@ -115,8 +115,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
 	struct ldlm_resource *res = req->l_resource;
 	struct ldlm_namespace *ns = ldlm_res_to_ns(res);
 	struct ldlm_lock *tmp;
-	struct ldlm_lock *ownlocks = NULL;
-	struct ldlm_lock *lock = NULL;
+	struct ldlm_lock *lock;
 	struct ldlm_lock *new = req;
 	struct ldlm_lock *new2 = NULL;
 	enum ldlm_mode mode = req->l_req_mode;
@@ -140,22 +139,14 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
 	/* This loop determines where this processes locks start
 	 * in the resource lr_granted list.
 	 */
-	list_for_each_entry(lock, &res->lr_granted, l_res_link) {
-		if (ldlm_same_flock_owner(lock, req)) {
-			ownlocks = lock;
+	list_for_each_entry(lock, &res->lr_granted, l_res_link)
+		if (ldlm_same_flock_owner(lock, req))
 			break;
-		}
-	}
 
 	/* Scan the locks owned by this process to find the insertion point
 	 * (as locks are ordered), and to handle overlaps.
 	 * We may have to merge or split existing locks.
 	 */
-	if (ownlocks)
-		lock = ownlocks;
-	else
-		lock = list_entry(&res->lr_granted,
-				  struct ldlm_lock, l_res_link);
 	list_for_each_entry_safe_from(lock, tmp, &res->lr_granted, l_res_link) {
 
 		if (!ldlm_same_flock_owner(lock, new))

[toc] | [prev] | [next] | [standalone]

#1690827 — [PATCH 05/12] staging: lustre: ldlm: remove unused 'work_list' arg from ldlm_process_flock_lock()

'work_list' is only set to NULL, and is never used.
So discard it.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
index 8ba3eaf49c65..dce81fbf3049 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
@@ -123,8 +123,7 @@ ldlm_flock_destroy(struct ldlm_lock *lock, enum ldlm_mode mode, __u64 flags)
  *
  */
 static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
-				   enum ldlm_error *err,
-				   struct list_head *work_list)
+				   enum ldlm_error *err)
 {
 	struct ldlm_resource *res = req->l_resource;
 	struct ldlm_namespace *ns = ldlm_res_to_ns(res);
@@ -594,7 +593,7 @@ ldlm_flock_completion_ast(struct ldlm_lock *lock, __u64 flags, void *data)
 		/* We need to reprocess the lock to do merges or splits
 		 * with existing locks owned by this process.
 		 */
-		ldlm_process_flock_lock(lock, &noreproc, &err, NULL);
+		ldlm_process_flock_lock(lock, &noreproc, &err);
 	}
 	unlock_res_and_lock(lock);
 	return rc;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web