Patchset: Simplify open_file_ntcreate
Volker Lendecke
Volker.Lendecke at SerNet.DE
Tue Oct 15 08:16:57 MDT 2013
Hi!
Attached find a patchset that simplifies open_file_ntcreate.
The main simplification is in the last patch: We only call
delay_for_oplocks once instead of twice. For my taste, this
significantly simplifies the flow of the routine. Thanks to
metze for the relevant insight!
Please review&push!
Thanks,
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 966c6d3dc967b7810f48921412dc46eee3c1e56b Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 2 Sep 2013 12:33:40 +0000
Subject: [PATCH 1/9] smbd: Simplify find_oplock_types a bit
Define a variable to dereference lck->data just once. Believe it or not,
this saves a few bytes .o with -O3 :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 93b69d5..29c6eb5 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1217,6 +1217,7 @@ static void find_oplock_types(files_struct *fsp,
bool *got_level2,
bool *got_no_oplock)
{
+ struct share_mode_data *d = lck->data;
int i;
*pp_batch = NULL;
@@ -1232,8 +1233,8 @@ static void find_oplock_types(files_struct *fsp,
return;
}
- for (i=0; i<lck->data->num_share_modes; i++) {
- struct share_mode_entry *e = &lck->data->share_modes[i];
+ for (i=0; i<d->num_share_modes; i++) {
+ struct share_mode_entry *e = &d->share_modes[i];
if (!is_valid_share_mode_entry(e)) {
continue;
@@ -1248,7 +1249,7 @@ static void find_oplock_types(files_struct *fsp,
if (BATCH_OPLOCK_TYPE(e->op_type)) {
/* batch - can only be one. */
- if (share_mode_stale_pid(lck->data, i)) {
+ if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale batch oplock\n"));
continue;
}
@@ -1259,7 +1260,7 @@ static void find_oplock_types(files_struct *fsp,
}
if (EXCLUSIVE_OPLOCK_TYPE(e->op_type)) {
- if (share_mode_stale_pid(lck->data, i)) {
+ if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale duplicate oplock\n"));
continue;
}
@@ -1272,7 +1273,7 @@ static void find_oplock_types(files_struct *fsp,
if (LEVEL_II_OPLOCK_TYPE(e->op_type)) {
if (*pp_batch || *pp_ex_or_batch) {
- if (share_mode_stale_pid(lck->data, i)) {
+ if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale LevelII "
"oplock\n"));
continue;
@@ -1284,7 +1285,7 @@ static void find_oplock_types(files_struct *fsp,
if (e->op_type == NO_OPLOCK) {
if (*pp_batch || *pp_ex_or_batch) {
- if (share_mode_stale_pid(lck->data, i)) {
+ if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale NO_OPLOCK "
"entry\n"));
continue;
--
1.7.9.5
From 0480f7557794a5aa856c4df05c8bd4b5b8ee6075 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 20 Aug 2013 11:58:15 +0000
Subject: [PATCH 2/9] smbd: Unify delay_for_*_oplocks
This is the same code in both routines
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 58 ++++++++++++---------------------------------------
1 file changed, 13 insertions(+), 45 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 29c6eb5..fa7c4a0 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1297,19 +1297,19 @@ static void find_oplock_types(files_struct *fsp,
}
}
-static bool delay_for_batch_oplocks(files_struct *fsp,
- uint64_t mid,
- int oplock_request,
- struct share_mode_entry *batch_entry)
+static bool delay_for_oplock(files_struct *fsp,
+ uint64_t mid,
+ int oplock_request,
+ struct share_mode_entry *entry)
{
if ((oplock_request & INTERNAL_OPEN_ONLY) || is_stat_open(fsp->access_mask)) {
return false;
}
- if (batch_entry == NULL) {
+ if (entry == NULL) {
return false;
}
- if (server_id_is_disconnected(&batch_entry->pid)) {
+ if (server_id_is_disconnected(&entry->pid)) {
/*
* TODO: clean up.
* This could be achieved by sending a break message
@@ -1322,33 +1322,7 @@ static bool delay_for_batch_oplocks(files_struct *fsp,
return false;
}
- /* Found a batch oplock */
- send_break_message(fsp, batch_entry, mid, oplock_request);
- return true;
-}
-
-static bool delay_for_exclusive_oplocks(files_struct *fsp,
- uint64_t mid,
- int oplock_request,
- struct share_mode_entry *ex_entry)
-{
- if ((oplock_request & INTERNAL_OPEN_ONLY) || is_stat_open(fsp->access_mask)) {
- return false;
- }
- if (ex_entry == NULL) {
- return false;
- }
-
- if (server_id_is_disconnected(&ex_entry->pid)) {
- /*
- * since only durable handles can get disconnected,
- * and we can only get durable handles with batch oplocks,
- * this should actually never be reached...
- */
- return false;
- }
-
- send_break_message(fsp, ex_entry, mid, oplock_request);
+ send_break_message(fsp, entry, mid, oplock_request);
return true;
}
@@ -2313,9 +2287,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
find_oplock_types(fsp, 0, lck, &batch_entry, &exclusive_entry,
&got_level2_oplock, &got_a_none_oplock);
- if (delay_for_batch_oplocks(fsp, req->mid, 0, batch_entry) ||
- delay_for_exclusive_oplocks(fsp, req->mid, 0,
- exclusive_entry)) {
+ if (delay_for_oplock(fsp, req->mid, 0, batch_entry) ||
+ delay_for_oplock(fsp, req->mid, 0, exclusive_entry)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
DEBUG(10, ("Sent oplock break request to kernel "
@@ -2415,10 +2388,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* First pass - send break only on batch oplocks. */
if ((req != NULL) &&
- delay_for_batch_oplocks(fsp,
- req->mid,
- oplock_request,
- batch_entry)) {
+ delay_for_oplock(fsp, req->mid, oplock_request,
+ batch_entry)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
fd_close(fsp);
@@ -2435,11 +2406,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* Second pass - send break for both batch or
* exclusive oplocks. */
if ((req != NULL) &&
- delay_for_exclusive_oplocks(
- fsp,
- req->mid,
- oplock_request,
- exclusive_entry)) {
+ delay_for_oplock(fsp, req->mid, oplock_request,
+ exclusive_entry)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
fd_close(fsp);
--
1.7.9.5
From 751aae3f104388b05ba451ccb53d45f24b2f6b17 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 15 Oct 2013 09:56:21 +0000
Subject: [PATCH 3/9] smbd: Make loop index type match loop limit
share_mode_data.num_share_modes is a uint32.
48 bytes less in .o text size for -O3 :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index fa7c4a0..9c8b31d 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1218,7 +1218,7 @@ static void find_oplock_types(files_struct *fsp,
bool *got_no_oplock)
{
struct share_mode_data *d = lck->data;
- int i;
+ uint32_t i;
*pp_batch = NULL;
*pp_ex_or_batch = NULL;
--
1.7.9.5
From ad7224f78c0510ef3a1c0fe00ede695ff9ce5262 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 15 Oct 2013 10:01:54 +0000
Subject: [PATCH 4/9] smbd: Make find_oplock_types return bool
smb_panic() does not take a printf style argument. This improves debug
output by easily printing the index that we fell over. Also, doing
smb_panic deep down is bad style IMHO.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 9c8b31d..a893c59 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1209,7 +1209,7 @@ static NTSTATUS send_break_message(files_struct *fsp,
* Do internal consistency checks on the share mode for a file.
*/
-static void find_oplock_types(files_struct *fsp,
+static bool find_oplock_types(files_struct *fsp,
int oplock_request,
const struct share_mode_lock *lck,
struct share_mode_entry **pp_batch,
@@ -1230,7 +1230,7 @@ static void find_oplock_types(files_struct *fsp,
delay_for_exclusive_oplocks().
*/
if ((oplock_request & INTERNAL_OPEN_ONLY) || is_stat_open(fsp->access_mask)) {
- return;
+ return true;
}
for (i=0; i<d->num_share_modes; i++) {
@@ -1254,7 +1254,9 @@ static void find_oplock_types(files_struct *fsp,
continue;
}
if (*pp_ex_or_batch || *pp_batch || *got_level2 || *got_no_oplock) {
- smb_panic("Bad batch oplock entry.");
+ DEBUG(0, ("Bad batch oplock entry %u.",
+ (unsigned)i));
+ return false;
}
*pp_batch = e;
}
@@ -1266,7 +1268,9 @@ static void find_oplock_types(files_struct *fsp,
}
/* Exclusive or batch - can only be one. */
if (*pp_ex_or_batch || *got_level2 || *got_no_oplock) {
- smb_panic("Bad exclusive or batch oplock entry.");
+ DEBUG(0, ("Bad exclusive or batch oplock "
+ "entry %u.", (unsigned)i));
+ return false;
}
*pp_ex_or_batch = e;
}
@@ -1278,7 +1282,9 @@ static void find_oplock_types(files_struct *fsp,
"oplock\n"));
continue;
}
- smb_panic("Bad levelII oplock entry.");
+ DEBUG(0, ("Bad levelII oplock entry %u.",
+ (unsigned)i));
+ return false;
}
*got_level2 = true;
}
@@ -1290,11 +1296,14 @@ static void find_oplock_types(files_struct *fsp,
"entry\n"));
continue;
}
- smb_panic("Bad no oplock entry.");
+ DEBUG(0, ("Bad no oplock entry %u.",
+ (unsigned)i));
+ return false;
}
*got_no_oplock = true;
}
}
+ return true;
}
static bool delay_for_oplock(files_struct *fsp,
@@ -2284,8 +2293,12 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
return NT_STATUS_SHARING_VIOLATION;
}
- find_oplock_types(fsp, 0, lck, &batch_entry, &exclusive_entry,
- &got_level2_oplock, &got_a_none_oplock);
+ if (!find_oplock_types(fsp, 0, lck,
+ &batch_entry, &exclusive_entry,
+ &got_level2_oplock,
+ &got_a_none_oplock)) {
+ smb_panic("find_oplock_types failed");
+ }
if (delay_for_oplock(fsp, req->mid, 0, batch_entry) ||
delay_for_oplock(fsp, req->mid, 0, exclusive_entry)) {
@@ -2372,13 +2385,12 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
}
/* Get the types we need to examine. */
- find_oplock_types(fsp,
- oplock_request,
- lck,
- &batch_entry,
- &exclusive_entry,
- &got_level2_oplock,
- &got_a_none_oplock);
+ if (!find_oplock_types(fsp, oplock_request, lck,
+ &batch_entry, &exclusive_entry,
+ &got_level2_oplock,
+ &got_a_none_oplock)) {
+ smb_panic("find_oplock_types failed");
+ }
if (has_delete_on_close(lck, fsp->name_hash)) {
TALLOC_FREE(lck);
--
1.7.9.5
From 68de7b5027e203dcb68c05d298b3eef19033f8cc Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 2 Sep 2013 11:37:57 +0000
Subject: [PATCH 5/9] smbd: Factor out remove_stale_share_mode_entries
Will be used in the next commit
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/locking/locking.c | 16 ++++++++++++++++
source3/locking/proto.h | 1 +
source3/locking/share_mode_lock.c | 15 +--------------
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index 7ac04a4..b5d4f24 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -693,6 +693,22 @@ bool share_mode_stale_pid(struct share_mode_data *d, uint32_t idx)
return true;
}
+void remove_stale_share_mode_entries(struct share_mode_data *d)
+{
+ uint32_t i;
+
+ i = 0;
+ while (i < d->num_share_modes) {
+ if (d->share_modes[i].stale) {
+ struct share_mode_entry *m = d->share_modes;
+ m[i] = m[d->num_share_modes-1];
+ d->num_share_modes -= 1;
+ } else {
+ i += 1;
+ }
+ }
+}
+
bool set_share_mode(struct share_mode_lock *lck, files_struct *fsp,
uid_t uid, uint64_t mid, uint16 op_type)
{
diff --git a/source3/locking/proto.h b/source3/locking/proto.h
index f6ae462..2c9654c 100644
--- a/source3/locking/proto.h
+++ b/source3/locking/proto.h
@@ -176,6 +176,7 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e);
bool share_mode_stale_pid(struct share_mode_data *d, uint32_t idx);
bool set_share_mode(struct share_mode_lock *lck, files_struct *fsp,
uid_t uid, uint64_t mid, uint16 op_type);
+void remove_stale_share_mode_entries(struct share_mode_data *d);
bool del_share_mode(struct share_mode_lock *lck, files_struct *fsp);
bool mark_share_mode_disconnected(struct share_mode_lock *lck,
struct files_struct *fsp);
diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
index 342f910..5d7a08c 100644
--- a/source3/locking/share_mode_lock.c
+++ b/source3/locking/share_mode_lock.c
@@ -171,26 +171,13 @@ static TDB_DATA unparse_share_modes(struct share_mode_data *d)
{
DATA_BLOB blob;
enum ndr_err_code ndr_err;
- uint32_t i;
if (DEBUGLEVEL >= 10) {
DEBUG(10, ("unparse_share_modes:\n"));
NDR_PRINT_DEBUG(share_mode_data, d);
}
- i = 0;
- while (i < d->num_share_modes) {
- if (d->share_modes[i].stale) {
- /*
- * Remove the stale entries before storing
- */
- struct share_mode_entry *m = d->share_modes;
- m[i] = m[d->num_share_modes-1];
- d->num_share_modes -= 1;
- } else {
- i += 1;
- }
- }
+ remove_stale_share_mode_entries(d);
if (d->num_share_modes == 0) {
DEBUG(10, ("No used share mode found\n"));
--
1.7.9.5
From 4c0c4173b9cfc67a71de394758799dc480972de8 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 2 Sep 2013 12:25:07 +0000
Subject: [PATCH 6/9] smbd: Reduce the complexity of open_file_ntcreate
This removes two variables in open_file_ntcreate based on the observation
that for exclusive and batch oplocks there can only be one entry. So
in these cases we don't need to keep pointers from find_oplock_types to
delay_for_oplocks. We can just reference the only share mode entry around.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 64 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 24 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index a893c59..cd8e2a0 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1212,16 +1212,16 @@ static NTSTATUS send_break_message(files_struct *fsp,
static bool find_oplock_types(files_struct *fsp,
int oplock_request,
const struct share_mode_lock *lck,
- struct share_mode_entry **pp_batch,
- struct share_mode_entry **pp_ex_or_batch,
bool *got_level2,
bool *got_no_oplock)
{
struct share_mode_data *d = lck->data;
+ bool batch = false;
+ bool ex_or_batch = false;
+ bool level2 = false;
+ bool no_oplock = false;
uint32_t i;
- *pp_batch = NULL;
- *pp_ex_or_batch = NULL;
*got_level2 = false;
*got_no_oplock = false;
@@ -1253,12 +1253,12 @@ static bool find_oplock_types(files_struct *fsp,
DEBUG(10, ("Found stale batch oplock\n"));
continue;
}
- if (*pp_ex_or_batch || *pp_batch || *got_level2 || *got_no_oplock) {
+ if (ex_or_batch || batch || level2 || no_oplock) {
DEBUG(0, ("Bad batch oplock entry %u.",
(unsigned)i));
return false;
}
- *pp_batch = e;
+ batch = true;
}
if (EXCLUSIVE_OPLOCK_TYPE(e->op_type)) {
@@ -1267,16 +1267,16 @@ static bool find_oplock_types(files_struct *fsp,
continue;
}
/* Exclusive or batch - can only be one. */
- if (*pp_ex_or_batch || *got_level2 || *got_no_oplock) {
+ if (ex_or_batch || level2 || no_oplock) {
DEBUG(0, ("Bad exclusive or batch oplock "
"entry %u.", (unsigned)i));
return false;
}
- *pp_ex_or_batch = e;
+ ex_or_batch = true;
}
if (LEVEL_II_OPLOCK_TYPE(e->op_type)) {
- if (*pp_batch || *pp_ex_or_batch) {
+ if (batch || ex_or_batch) {
if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale LevelII "
"oplock\n"));
@@ -1286,11 +1286,11 @@ static bool find_oplock_types(files_struct *fsp,
(unsigned)i));
return false;
}
- *got_level2 = true;
+ level2 = true;
}
if (e->op_type == NO_OPLOCK) {
- if (*pp_batch || *pp_ex_or_batch) {
+ if (batch || ex_or_batch) {
if (share_mode_stale_pid(d, i)) {
DEBUG(10, ("Found stale NO_OPLOCK "
"entry\n"));
@@ -1300,21 +1300,41 @@ static bool find_oplock_types(files_struct *fsp,
(unsigned)i));
return false;
}
- *got_no_oplock = true;
+ no_oplock = true;
}
}
+
+ remove_stale_share_mode_entries(d);
+
+ if ((batch || ex_or_batch) && (d->num_share_modes != 1)) {
+ DEBUG(1, ("got batch (%d) or ex (%d) non-exclusively (%d)\n",
+ (int)batch, (int)ex_or_batch,
+ (int)d->num_share_modes));
+ return false;
+ }
+
+ *got_level2 = level2;
+ *got_no_oplock = no_oplock;
return true;
}
static bool delay_for_oplock(files_struct *fsp,
uint64_t mid,
int oplock_request,
- struct share_mode_entry *entry)
+ struct share_mode_lock *lck,
+ int delay_for_type)
{
+ struct share_mode_entry *entry;
+
if ((oplock_request & INTERNAL_OPEN_ONLY) || is_stat_open(fsp->access_mask)) {
return false;
}
- if (entry == NULL) {
+ if (lck->data->num_share_modes != 1) {
+ return false;
+ }
+ entry = &lck->data->share_modes[0];
+
+ if (!(entry->op_type & delay_for_type)) {
return false;
}
@@ -1949,8 +1969,6 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
NTSTATUS status;
char *parent_dir;
SMB_STRUCT_STAT saved_stat = smb_fname->st;
- struct share_mode_entry *batch_entry = NULL;
- struct share_mode_entry *exclusive_entry = NULL;
bool got_level2_oplock = false;
bool got_a_none_oplock = false;
struct timespec old_write_time;
@@ -2294,14 +2312,13 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
}
if (!find_oplock_types(fsp, 0, lck,
- &batch_entry, &exclusive_entry,
&got_level2_oplock,
&got_a_none_oplock)) {
smb_panic("find_oplock_types failed");
}
- if (delay_for_oplock(fsp, req->mid, 0, batch_entry) ||
- delay_for_oplock(fsp, req->mid, 0, exclusive_entry)) {
+ if (delay_for_oplock(fsp, req->mid, 0, lck,
+ BATCH_OPLOCK|EXCLUSIVE_OPLOCK)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
DEBUG(10, ("Sent oplock break request to kernel "
@@ -2386,7 +2403,6 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* Get the types we need to examine. */
if (!find_oplock_types(fsp, oplock_request, lck,
- &batch_entry, &exclusive_entry,
&got_level2_oplock,
&got_a_none_oplock)) {
smb_panic("find_oplock_types failed");
@@ -2400,8 +2416,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* First pass - send break only on batch oplocks. */
if ((req != NULL) &&
- delay_for_oplock(fsp, req->mid, oplock_request,
- batch_entry)) {
+ delay_for_oplock(fsp, req->mid, oplock_request, lck,
+ BATCH_OPLOCK)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
fd_close(fsp);
@@ -2418,8 +2434,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* Second pass - send break for both batch or
* exclusive oplocks. */
if ((req != NULL) &&
- delay_for_oplock(fsp, req->mid, oplock_request,
- exclusive_entry)) {
+ delay_for_oplock(fsp, req->mid, oplock_request, lck,
+ BATCH_OPLOCK|EXCLUSIVE_OPLOCK)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
fd_close(fsp);
--
1.7.9.5
From c3164cd28234053b8c2e6dd50b2022d331228eb0 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 3 Sep 2013 09:02:12 +0000
Subject: [PATCH 7/9] smbd: Decouple grant_fsp_oplock_type from oplock
validation
This makes grant_fsp_oplock_type independent from the values computed
in validate_oplock_types. It *might* make oplock calculation a bit
slower for heavily shared files, as we are walking the share mode array
twice. But we are doing so much stuff in open that I doubt the difference
is measurable. It clears up the code for me however, and I think that's
worth it.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 53 +++++++++++++++++++++++++--------------------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index cd8e2a0..670de5d 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1209,11 +1209,9 @@ static NTSTATUS send_break_message(files_struct *fsp,
* Do internal consistency checks on the share mode for a file.
*/
-static bool find_oplock_types(files_struct *fsp,
- int oplock_request,
- const struct share_mode_lock *lck,
- bool *got_level2,
- bool *got_no_oplock)
+static bool validate_oplock_types(files_struct *fsp,
+ int oplock_request,
+ struct share_mode_lock *lck)
{
struct share_mode_data *d = lck->data;
bool batch = false;
@@ -1222,9 +1220,6 @@ static bool find_oplock_types(files_struct *fsp,
bool no_oplock = false;
uint32_t i;
- *got_level2 = false;
- *got_no_oplock = false;
-
/* Ignore stat or internal opens, as is done in
delay_for_batch_oplocks() and
delay_for_exclusive_oplocks().
@@ -1313,8 +1308,6 @@ static bool find_oplock_types(files_struct *fsp,
return false;
}
- *got_level2 = level2;
- *got_no_oplock = no_oplock;
return true;
}
@@ -1367,12 +1360,13 @@ static bool file_has_brlocks(files_struct *fsp)
}
static void grant_fsp_oplock_type(files_struct *fsp,
- int oplock_request,
- bool got_level2_oplock,
- bool got_a_none_oplock)
+ struct share_mode_lock *lck,
+ int oplock_request)
{
bool allow_level2 = (global_client_caps & CAP_LEVEL_II_OPLOCKS) &&
lp_level2_oplocks(SNUM(fsp->conn));
+ bool got_level2_oplock, got_a_none_oplock;
+ uint32_t i;
/* Start by granting what the client asked for,
but ensure no SAMBA_PRIVATE bits can be set. */
@@ -1399,6 +1393,20 @@ static void grant_fsp_oplock_type(files_struct *fsp,
return;
}
+ got_level2_oplock = false;
+ got_a_none_oplock = false;
+
+ for (i=0; i<lck->data->num_share_modes; i++) {
+ int op_type = lck->data->share_modes[i].op_type;
+
+ if (LEVEL_II_OPLOCK_TYPE(op_type)) {
+ got_level2_oplock = true;
+ }
+ if (op_type == NO_OPLOCK) {
+ got_a_none_oplock = true;
+ }
+ }
+
/*
* Match what was requested (fsp->oplock_type) with
* what was found in the existing share modes.
@@ -1969,8 +1977,6 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
NTSTATUS status;
char *parent_dir;
SMB_STRUCT_STAT saved_stat = smb_fname->st;
- bool got_level2_oplock = false;
- bool got_a_none_oplock = false;
struct timespec old_write_time;
struct file_id id;
@@ -2311,10 +2317,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
return NT_STATUS_SHARING_VIOLATION;
}
- if (!find_oplock_types(fsp, 0, lck,
- &got_level2_oplock,
- &got_a_none_oplock)) {
- smb_panic("find_oplock_types failed");
+ if (!validate_oplock_types(fsp, 0, lck)) {
+ smb_panic("validate_oplock_types failed");
}
if (delay_for_oplock(fsp, req->mid, 0, lck,
@@ -2402,10 +2406,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
}
/* Get the types we need to examine. */
- if (!find_oplock_types(fsp, oplock_request, lck,
- &got_level2_oplock,
- &got_a_none_oplock)) {
- smb_panic("find_oplock_types failed");
+ if (!validate_oplock_types(fsp, oplock_request, lck)) {
+ smb_panic("validate_oplock_types failed");
}
if (has_delete_on_close(lck, fsp->name_hash)) {
@@ -2570,10 +2572,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
return status;
}
- grant_fsp_oplock_type(fsp,
- oplock_request,
- got_level2_oplock,
- got_a_none_oplock);
+ grant_fsp_oplock_type(fsp, lck, oplock_request);
/*
* We have the share entry *locked*.....
--
1.7.9.5
From 84fff7c95f1f53e2111785cd231d874f2e59bbc8 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 26 Sep 2013 14:32:09 -0700
Subject: [PATCH 8/9] smbd: Remove "file_existed" handling from
open_mode_check
No clue what this does. In open_directory, "dir_existed" is not used after
open_mode_check. In open_file_ntcreate it's used, but I can't think of a case
right now where we would find a formerly nonexisting file to exist suddenly.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 670de5d..aaa42b3 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1114,8 +1114,7 @@ static bool has_delete_on_close(struct share_mode_lock *lck,
static NTSTATUS open_mode_check(connection_struct *conn,
struct share_mode_lock *lck,
uint32 access_mask,
- uint32 share_access,
- bool *file_existed)
+ uint32 share_access)
{
int i;
@@ -1156,16 +1155,10 @@ static NTSTATUS open_mode_check(connection_struct *conn,
continue;
}
- *file_existed = true;
-
return NT_STATUS_SHARING_VIOLATION;
}
}
- if (lck->data->num_share_modes != 0) {
- *file_existed = true;
- }
-
return NT_STATUS_OK;
}
@@ -2427,8 +2420,20 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
}
status = open_mode_check(conn, lck,
- access_mask, share_access,
- &file_existed);
+ access_mask, share_access);
+
+
+ if (NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION) ||
+ (lck->data->num_share_modes > 0)) {
+ /*
+ * This comes from ancient times out of open_mode_check. I
+ * have no clue whether this is still necessary. I can't think
+ * of a case where this would actually matter further down in
+ * this function. I leave it here for further investigation
+ * :-)
+ */
+ file_existed = true;
+ }
if (NT_STATUS_IS_OK(status)) {
/* We might be going to allow this open. Check oplock
@@ -3159,8 +3164,7 @@ static NTSTATUS open_directory(connection_struct *conn,
}
status = open_mode_check(conn, lck,
- access_mask, share_access,
- &dir_existed);
+ access_mask, share_access);
if (!NT_STATUS_IS_OK(status)) {
TALLOC_FREE(lck);
--
1.7.9.5
From f13c5d49d07572bd59cff72279a7aa8600caff4b Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 26 Sep 2013 15:20:36 -0700
Subject: [PATCH 9/9] smbd: Simplify open_file_ntcreate
Only one call to delay_for_oplocks left. Metze showed me the new logic:
BATCH is broken if we have a sharing violation. Exclusive is broken
otherwise. That's it.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/open.c | 71 ++++++++++++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index aaa42b3..fa52fcc 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1308,21 +1308,21 @@ static bool delay_for_oplock(files_struct *fsp,
uint64_t mid,
int oplock_request,
struct share_mode_lock *lck,
- int delay_for_type)
+ bool have_sharing_violation)
{
+ struct share_mode_data *d = lck->data;
struct share_mode_entry *entry;
if ((oplock_request & INTERNAL_OPEN_ONLY) || is_stat_open(fsp->access_mask)) {
return false;
}
if (lck->data->num_share_modes != 1) {
+ /*
+ * More than one. There can't be any exclusive or batch left.
+ */
return false;
}
- entry = &lck->data->share_modes[0];
-
- if (!(entry->op_type & delay_for_type)) {
- return false;
- }
+ entry = &d->share_modes[0];
if (server_id_is_disconnected(&entry->pid)) {
/*
@@ -1337,6 +1337,30 @@ static bool delay_for_oplock(files_struct *fsp,
return false;
}
+ if (have_sharing_violation && (entry->op_type & BATCH_OPLOCK)) {
+ if (share_mode_stale_pid(d, 0)) {
+ return false;
+ }
+ send_break_message(fsp, entry, mid, oplock_request);
+ return true;
+ }
+ if (have_sharing_violation) {
+ /*
+ * Non-batch exclusive is not broken if we have a sharing
+ * violation
+ */
+ return false;
+ }
+ if (!EXCLUSIVE_OPLOCK_TYPE(entry->op_type)) {
+ /*
+ * No break for NO_OPLOCK or LEVEL2_OPLOCK oplocks
+ */
+ return false;
+ }
+ if (share_mode_stale_pid(d, 0)) {
+ return false;
+ }
+
send_break_message(fsp, entry, mid, oplock_request);
return true;
}
@@ -2314,8 +2338,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
smb_panic("validate_oplock_types failed");
}
- if (delay_for_oplock(fsp, req->mid, 0, lck,
- BATCH_OPLOCK|EXCLUSIVE_OPLOCK)) {
+ if (delay_for_oplock(fsp, req->mid, 0, lck, false)) {
schedule_defer_open(lck, request_time, req);
TALLOC_FREE(lck);
DEBUG(10, ("Sent oplock break request to kernel "
@@ -2409,20 +2432,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
return NT_STATUS_DELETE_PENDING;
}
- /* First pass - send break only on batch oplocks. */
- if ((req != NULL) &&
- delay_for_oplock(fsp, req->mid, oplock_request, lck,
- BATCH_OPLOCK)) {
- schedule_defer_open(lck, request_time, req);
- TALLOC_FREE(lck);
- fd_close(fsp);
- return NT_STATUS_SHARING_VIOLATION;
- }
-
status = open_mode_check(conn, lck,
access_mask, share_access);
-
if (NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION) ||
(lck->data->num_share_modes > 0)) {
/*
@@ -2435,19 +2447,14 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
file_existed = true;
}
- if (NT_STATUS_IS_OK(status)) {
- /* We might be going to allow this open. Check oplock
- * status again. */
- /* Second pass - send break for both batch or
- * exclusive oplocks. */
- if ((req != NULL) &&
- delay_for_oplock(fsp, req->mid, oplock_request, lck,
- BATCH_OPLOCK|EXCLUSIVE_OPLOCK)) {
- schedule_defer_open(lck, request_time, req);
- TALLOC_FREE(lck);
- fd_close(fsp);
- return NT_STATUS_SHARING_VIOLATION;
- }
+ if ((req != NULL) &&
+ delay_for_oplock(
+ fsp, req->mid, oplock_request, lck,
+ NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION))) {
+ schedule_defer_open(lck, request_time, req);
+ TALLOC_FREE(lck);
+ fd_close(fsp);
+ return NT_STATUS_SHARING_VIOLATION;
}
if (!NT_STATUS_IS_OK(status)) {
--
1.7.9.5
More information about the samba-technical
mailing list