[1/3] CIFS: Fix tcon and file locking
diff mbox

Message ID 1476225247-30177-1-git-send-email-piastryyy@gmail.com
State New
Headers show

Commit Message

Pavel Shilovsky Oct. 11, 2016, 10:34 p.m. UTC
From: Pavel Shilovsky <pshilov@microsoft.com>

This fix is supposed to be squashed into
"Clarify locking of cifs file and tcon structures and make more granular".

Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/file.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Steve French Oct. 12, 2016, 3:26 a.m. UTC | #1
patch merged in with the patch noted above (and the other two patches
from Pavel also merged) into cifs-2.6.git for-next

On Tue, Oct 11, 2016 at 5:34 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> From: Pavel Shilovsky <pshilov@microsoft.com>
>
> This fix is supposed to be squashed into
> "Clarify locking of cifs file and tcon structures and make more granular".
>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>  fs/cifs/file.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 1e7c625..20801dc 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -318,14 +318,14 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>                 oplock = 0;
>         }
>
> +       spin_lock(&tcon->open_file_lock);
>         if (fid->pending_open->oplock != CIFS_OPLOCK_NO_CHANGE && oplock)
>                 oplock = fid->pending_open->oplock;
> -       cifs_del_pending_open(fid->pending_open);
> +       list_del(&fid->pending_open->olist);
>
>         fid->purge_cache = false;
>         server->ops->set_fid(cfile, fid, oplock);
>
> -       spin_lock(&tcon->open_file_lock);
>         list_add(&cfile->tlist, &tcon->openFileList);
>
>         /* if readable file instance put first in list*/
> @@ -354,7 +354,7 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file)
>  /*
>   * Release a reference on the file private data. This may involve closing
>   * the filehandle out on the server. Must be called without holding
> - * cifs_file->file_info_lock.
> + * tcon->open_file_lock and cifs_file->file_info_lock.
>   */
>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  {
> @@ -369,11 +369,15 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>         struct cifs_pending_open open;
>         bool oplock_break_cancelled;
>
> +       spin_lock(&tcon->open_file_lock);
> +
>         spin_lock(&cifs_file->file_info_lock);
>         if (--cifs_file->count > 0) {
>                 spin_unlock(&cifs_file->file_info_lock);
> +               spin_unlock(&tcon->open_file_lock);
>                 return;
>         }
> +       spin_unlock(&cifs_file->file_info_lock);
>
>         if (server->ops->get_lease_key)
>                 server->ops->get_lease_key(inode, &fid);
> @@ -397,7 +401,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>                         set_bit(CIFS_INO_INVALID_MAPPING, &cifsi->flags);
>                 cifs_set_oplock_level(cifsi, 0);
>         }
> -       spin_unlock(&cifs_file->file_info_lock);
> +
> +       spin_unlock(&tcon->open_file_lock);
>
>         oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break);
>
> @@ -1765,7 +1770,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
>                         if (!open_file->invalidHandle) {
>                                 /* found a good file */
>                                 /* lock it so it will not be closed on us */
> -                               cifsFileInfo_get_locked(open_file);
> +                               cifsFileInfo_get(open_file);
>                                 spin_unlock(&tcon->open_file_lock);
>                                 return open_file;
>                         } /* else might as well continue, and look for
> @@ -1819,7 +1824,7 @@ refind_writable:
>                 if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
>                         if (!open_file->invalidHandle) {
>                                 /* found a good writable file */
> -                               cifsFileInfo_get_locked(open_file);
> +                               cifsFileInfo_get(open_file);
>                                 spin_unlock(&tcon->open_file_lock);
>                                 return open_file;
>                         } else {
> @@ -1836,9 +1841,7 @@ refind_writable:
>
>         if (inv_file) {
>                 any_available = false;
> -               spin_lock(&inv_file->file_info_lock);
> -               cifsFileInfo_get_locked(inv_file);
> -               spin_unlock(&inv_file->file_info_lock);
> +               cifsFileInfo_get(inv_file);
>         }
>
>         spin_unlock(&tcon->open_file_lock);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Germano Percossi Oct. 17, 2016, 11:29 a.m. UTC | #2
I tested this patchset, along with the following on top
of 4.4 kernel, in our XenServer stress test (450
Virtual Machine life cycle operations with 2 different
SMB3 targets, Windows and Netapp).

The list of tested patches follows:

cifs-Limit-the-overall-credit-acquired
fix_memory_leaks_in_cifs_do_mount
compare_prepaths_when_comparing_superblocks
move_check_for_prefix_path_to_within_cifs_get_root
display_number_of_credits_available
set_previous_session_id_correctly_on_smb3_reconnect
smb3__guids_should_be_constructed_as_random_but_valid_uuids
fs-cifs__keep_guid_when_assigning_fid_to_fileinfo
fix_regression_which_breaks_dfs_mounting
clarify_locking_of_cifs_file_and_tcon_structures_and_make_more_granular
fs-cifs__reopen_persistent_handles_on_reconnect
expose_module_parameters_in_sysfs
smb3__add_mount_parameter_to_allow_user_to_override_max_credits
cifs__fix_persistent_handles_re-opening_on_reconnect
cifs__reset_read_oplock_to_none_if_we_have_mandatory_locks_after_reopen

Without the recent fixes to tcon_open_file_list loop, the stress test
failed (a temporary fix was to loop through it with the safe loop
alternative).

Overall the stress tests are passing.

Regards,
Germano

On 10/12/2016 04:26 AM, Steve French wrote:
> patch merged in with the patch noted above (and the other two patches
> from Pavel also merged) into cifs-2.6.git for-next
> 
> On Tue, Oct 11, 2016 at 5:34 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>> From: Pavel Shilovsky <pshilov@microsoft.com>
>>
>> This fix is supposed to be squashed into
>> "Clarify locking of cifs file and tcon structures and make more granular".
>>
>> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
>> ---
>>  fs/cifs/file.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 1e7c625..20801dc 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -318,14 +318,14 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>>                 oplock = 0;
>>         }
>>
>> +       spin_lock(&tcon->open_file_lock);
>>         if (fid->pending_open->oplock != CIFS_OPLOCK_NO_CHANGE && oplock)
>>                 oplock = fid->pending_open->oplock;
>> -       cifs_del_pending_open(fid->pending_open);
>> +       list_del(&fid->pending_open->olist);
>>
>>         fid->purge_cache = false;
>>         server->ops->set_fid(cfile, fid, oplock);
>>
>> -       spin_lock(&tcon->open_file_lock);
>>         list_add(&cfile->tlist, &tcon->openFileList);
>>
>>         /* if readable file instance put first in list*/
>> @@ -354,7 +354,7 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file)
>>  /*
>>   * Release a reference on the file private data. This may involve closing
>>   * the filehandle out on the server. Must be called without holding
>> - * cifs_file->file_info_lock.
>> + * tcon->open_file_lock and cifs_file->file_info_lock.
>>   */
>>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>  {
>> @@ -369,11 +369,15 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>         struct cifs_pending_open open;
>>         bool oplock_break_cancelled;
>>
>> +       spin_lock(&tcon->open_file_lock);
>> +
>>         spin_lock(&cifs_file->file_info_lock);
>>         if (--cifs_file->count > 0) {
>>                 spin_unlock(&cifs_file->file_info_lock);
>> +               spin_unlock(&tcon->open_file_lock);
>>                 return;
>>         }
>> +       spin_unlock(&cifs_file->file_info_lock);
>>
>>         if (server->ops->get_lease_key)
>>                 server->ops->get_lease_key(inode, &fid);
>> @@ -397,7 +401,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>                         set_bit(CIFS_INO_INVALID_MAPPING, &cifsi->flags);
>>                 cifs_set_oplock_level(cifsi, 0);
>>         }
>> -       spin_unlock(&cifs_file->file_info_lock);
>> +
>> +       spin_unlock(&tcon->open_file_lock);
>>
>>         oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break);
>>
>> @@ -1765,7 +1770,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
>>                         if (!open_file->invalidHandle) {
>>                                 /* found a good file */
>>                                 /* lock it so it will not be closed on us */
>> -                               cifsFileInfo_get_locked(open_file);
>> +                               cifsFileInfo_get(open_file);
>>                                 spin_unlock(&tcon->open_file_lock);
>>                                 return open_file;
>>                         } /* else might as well continue, and look for
>> @@ -1819,7 +1824,7 @@ refind_writable:
>>                 if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
>>                         if (!open_file->invalidHandle) {
>>                                 /* found a good writable file */
>> -                               cifsFileInfo_get_locked(open_file);
>> +                               cifsFileInfo_get(open_file);
>>                                 spin_unlock(&tcon->open_file_lock);
>>                                 return open_file;
>>                         } else {
>> @@ -1836,9 +1841,7 @@ refind_writable:
>>
>>         if (inv_file) {
>>                 any_available = false;
>> -               spin_lock(&inv_file->file_info_lock);
>> -               cifsFileInfo_get_locked(inv_file);
>> -               spin_unlock(&inv_file->file_info_lock);
>> +               cifsFileInfo_get(inv_file);
>>         }
>>
>>         spin_unlock(&tcon->open_file_lock);
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e7c625..20801dc 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -318,14 +318,14 @@  cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 		oplock = 0;
 	}
 
+	spin_lock(&tcon->open_file_lock);
 	if (fid->pending_open->oplock != CIFS_OPLOCK_NO_CHANGE && oplock)
 		oplock = fid->pending_open->oplock;
-	cifs_del_pending_open(fid->pending_open);
+	list_del(&fid->pending_open->olist);
 
 	fid->purge_cache = false;
 	server->ops->set_fid(cfile, fid, oplock);
 
-	spin_lock(&tcon->open_file_lock);
 	list_add(&cfile->tlist, &tcon->openFileList);
 
 	/* if readable file instance put first in list*/
@@ -354,7 +354,7 @@  cifsFileInfo_get(struct cifsFileInfo *cifs_file)
 /*
  * Release a reference on the file private data. This may involve closing
  * the filehandle out on the server. Must be called without holding
- * cifs_file->file_info_lock.
+ * tcon->open_file_lock and cifs_file->file_info_lock.
  */
 void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
@@ -369,11 +369,15 @@  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct cifs_pending_open open;
 	bool oplock_break_cancelled;
 
+	spin_lock(&tcon->open_file_lock);
+
 	spin_lock(&cifs_file->file_info_lock);
 	if (--cifs_file->count > 0) {
 		spin_unlock(&cifs_file->file_info_lock);
+		spin_unlock(&tcon->open_file_lock);
 		return;
 	}
+	spin_unlock(&cifs_file->file_info_lock);
 
 	if (server->ops->get_lease_key)
 		server->ops->get_lease_key(inode, &fid);
@@ -397,7 +401,8 @@  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 			set_bit(CIFS_INO_INVALID_MAPPING, &cifsi->flags);
 		cifs_set_oplock_level(cifsi, 0);
 	}
-	spin_unlock(&cifs_file->file_info_lock);
+
+	spin_unlock(&tcon->open_file_lock);
 
 	oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break);
 
@@ -1765,7 +1770,7 @@  struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 			if (!open_file->invalidHandle) {
 				/* found a good file */
 				/* lock it so it will not be closed on us */
-				cifsFileInfo_get_locked(open_file);
+				cifsFileInfo_get(open_file);
 				spin_unlock(&tcon->open_file_lock);
 				return open_file;
 			} /* else might as well continue, and look for
@@ -1819,7 +1824,7 @@  refind_writable:
 		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
 			if (!open_file->invalidHandle) {
 				/* found a good writable file */
-				cifsFileInfo_get_locked(open_file);
+				cifsFileInfo_get(open_file);
 				spin_unlock(&tcon->open_file_lock);
 				return open_file;
 			} else {
@@ -1836,9 +1841,7 @@  refind_writable:
 
 	if (inv_file) {
 		any_available = false;
-		spin_lock(&inv_file->file_info_lock);
-		cifsFileInfo_get_locked(inv_file);
-		spin_unlock(&inv_file->file_info_lock);
+		cifsFileInfo_get(inv_file);
 	}
 
 	spin_unlock(&tcon->open_file_lock);