cifs: accidentally creating empty files
Subsystems (2)
PATCH 1/1 cifs: accidentally creating empty files
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
fs/cifs/dir.c +4 -2
@@ -566,11 +566,13 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,if (nd && !(nd->flags &LOOKUP_DIRECTORY ) && (nd->flags &LOOKUP_OPEN ) && !pTcon->broken_posix_open && (nd->intent.open.file->f_flags &O_CREAT )) { +unsigned int f_flags; + + f_flags = (nd->intent.open.file->f_flags & ~O_CREAT ); rc =cifs_posix_open (full_path, &newInode, parent_dir_inode->i_sb, nd->intent.open.create_mode, - nd->intent.open.file->f_flags, &oplock, - &fileHandle, xid); + f_flags, &oplock, &fileHandle, xid);/* * The check below works around a bug in POSIX * open in samba versions 3.3.1 and earlier where
Quoted 4 lines · Dan Carpenter
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
I never heard back on this.
regards,
dan carpenter
Looking at it now - thanks for reminding me.
Anyone else review/ack the patch?
Quoted 12 lines · Dan Carpenter
Quoted 4 lines · Dan Carpenter
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
I never heard back on this.
regards,
dan carpenter
Signature · 3 lines
Thanks, Steve
Quoted 4 lines · On Wed, 28 Sep 2011 00:28:10 +0300 Dan Carpenter <[email protected]>…
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
Not quite. This uses the open-intent goop in the VFS layer that Al Viro is trying to get rid of. The idea here is that doing a lookup just to do an open is a waste, when you could just attempt the open. There's no real reason to exempt creates from that if cifs used a sane permissions model...
Quoted 9 lines
Signed-off-by Dan Carpenter <[email protected]>fs/cifs/dir.c +0
-0@@ -566,11 +566,13 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, if (nd && !(nd->flags & LOOKUP_DIRECTORY) && (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
fs/cifs/dir.c cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, · line 575
Quoted 10 lines
fs/cifs/dir.c +4
-2(nd->intent.open.file->f_flags & O_CREAT)) { +unsigned int f_flags; + + f_flags = (nd->intent.open.file->f_flags & ~O_CREAT ); rc =cifs_posix_open (full_path, &newInode, parent_dir_inode->i_sb, nd->intent.open.create_mode, - nd->intent.open.file->f_flags, &oplock, - &fileHandle, xid); + f_flags, &oplock, &fileHandle, xid);
...so this makes it only do the open at lookup time if the file already exists.
I suspect the real problem here is that cifs is trying to enforce permissions on the client, which happens after the lookup.
If the client simply allowed the server to handle the permissions (and used the right credentials for each user), then this would probably work fine. Another nail in the coffin for the whole model of client side permissions enforcement, IMO...
In any case, this seems fine to me, so...
fs/cifs/dir.c cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, · line 578
Quoted 3 lines
fs/cifs/dir.c +0
-0/* * The check below works around a bug in POSIX * open in samba versions 3.3.1 and earlier where
A message that is not in the archive (sent off-list or not yet received): 20111013132755.6c5db87e-xSBYVWDuneFaJnirhKH9O4GKTjYczspe@public.gmane.org
Quoted 42 lines · Jeff Layton
Quoted 4 lines · On Wed, 28 Sep 2011 00:28:10 +0300 Dan Carpenter…
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
Not quite. This uses the open-intent goop in the VFS layer that Al Viro is trying to get rid of. The idea here is that doing a lookup just to do an open is a waste, when you could just attempt the open. There's no real reason to exempt creates from that if cifs used a sane permissions model...
Quoted 11 lines
Signed-off-by Dan Carpenter <[email protected]>fs/cifs/dir.c +0
-0@@ -566,11 +566,13 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,if (nd && !(nd->flags &
LOOKUP_DIRECTORY) && (nd->flags&LOOKUP_OPEN) && !pTcon->broken_posix_open && (nd->intent.open.file->f_flags&O_CREAT)) {
+ unsigned intf_flags;Quoted 8 lines
+
+f_flags= (nd->intent.open.file->f_flags& ~O_CREAT); rc = cifs_posix_open(full_path, &newInode,parent_dir_inode->i_sb,nd->intent.open.create_mode,
-nd->intent.open.file->f_flags, &oplock,
- &fileHandle, xid);
+f_flags, &oplock, &fileHandle, xid);...so this makes it only do the open at lookup time if the file already exists.
I suspect the real problem here is that cifs is trying to enforce permissions on the client, which happens after the lookup.
Quoted 5 lines
If the client simply allowed the server to handle the permissions (and used the right credentials for each user), then this would probably work fine. Another nail in the coffin for the whole model of client side permissions enforcement, IMO...
so it works with "noperm" on. Presumably we should be recommending turning noperm and multiuser on (and probably cifsacl if the server is not samba).
I still would much prefer being able to hook in to the create and open "closer" to the syscall itself rather than after it is broken down into (unneeded) lower level fs entry points (for getattr/setattr and create vs. open).
--
Thanks,
Steve
Quoted 37 lines · Jeff Layton
Quoted 4 lines · On Wed, 28 Sep 2011 00:28:10 +0300 Dan Carpenter…
This solves a problem for where a user without write permissions was creating empty files. This function was supposed to do a lookup only, the create happens later.
Not quite. This uses the open-intent goop in the VFS layer that Al Viro is trying to get rid of. The idea here is that doing a lookup just to do an open is a waste, when you could just attempt the open. There's no real reason to exempt creates from that if cifs used a sane permissions model...
Quoted 11 lines
Signed-off-by Dan Carpenter <[email protected]>fs/cifs/dir.c +0
-0@@ -566,11 +566,13 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,if (nd && !(nd->flags &
LOOKUP_DIRECTORY) && (nd->flags&LOOKUP_OPEN) && !pTcon->broken_posix_open && (nd->intent.open.file->f_flags&O_CREAT)) {
+ unsigned intf_flags;Quoted 8 lines
+
+f_flags= (nd->intent.open.file->f_flags& ~O_CREAT); rc = cifs_posix_open(full_path, &newInode,parent_dir_inode->i_sb,nd->intent.open.create_mode,
-nd->intent.open.file->f_flags, &oplock,
- &fileHandle, xid);
+f_flags, &oplock, &fileHandle, xid);...so this makes it only do the open at lookup time if the file already exists.
Quoted 6 lines
I suspect the real problem here is that cifs is trying to enforce permissions on the client, which happens after the lookup.
If the client simply allowed the server to handle the permissions (and used the right credentials for each user),
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Can we fix this to (at least) narrow the performance penalty.
Signature · 3 lines
Thanks, Steve
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the O_CREAT flag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup. It's obviously not pretty but it might be worth it for speedup.
regards,
dan carpenter
A message that is not in the archive (sent off-list or not yet received): 20111028072019.GA14900-dZEljifmRObu9KfB+GxooP8+0UxHXcjY@public.gmane.org
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5msb_+tenSg1kWCn_0xi2EZu6vboqvmbQ17vRBYoqHp2Eg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French…
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.
Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
Signature · 1 line
Jeff Layton <[email protected]>
A message that is not in the archive (sent off-list or not yet received): 20111028234406.01fd23ba-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org
Quoted 39 lines · Jeff Layton
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French…
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
Quoted 3 lines
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
We don't have much choice - we have to allow permission checking on the client when client security context can't match security context on server - but I would like to make multiuser the default (especially if we can figure out a way to integrate winbind-like upcall for ntlmv2 auth) at least for the future (smb2 etc.) even if we can't change the default for cifs mounts.
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5mvHF0X6PQ8FyBfVEpQeyWgd7RGkawJykapkKt0K84QTHw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 44 lines · On Sat, 29 Oct 2011 00:07:12 -0500 Steve French…
Quoted 39 lines · Jeff Layton
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French…
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
Quoted 3 lines
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
Quoted 8 lines
We don't have much choice - we have to allow permission checking on the client when client security context can't match security context on server - but I would like to make multiuser the default (especially if we can figure out a way to integrate winbind-like upcall for ntlmv2 auth) at least for the future (smb2 etc.) even if we can't change the default for cifs mounts.
That's fine, but that doesn't solve Dan's immediate problem. We need to either take his patch or something like it for 3.2 (and probably for stable as well) or just rip this crap out altogether.
I'd favor the latter, for the following reason:
----------------------[snip]-------------------
/*
* O_EXCL: optimize away the lookup, but don't hash the dentry. Let
* the VFS handle the create.
*/
if (nd && (nd->flags & LOOKUP_EXCL)) {
d_instantiate(direntry, NULL);
rc = 0;
goto lookup_out;
}
[...]
if (pTcon->unix_ext) {
if (nd && !(nd->flags & LOOKUP_DIRECTORY) &&
(nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
(nd->intent.open.file->f_flags & O_CREAT)) {
rc = cifs_posix_open(full_path, &newInode,
parent_dir_inode->i_sb,
nd->intent.open.create_mode,
nd->intent.open.file->f_flags, &oplock,
&fileHandle, xid);----------------------[snip]-------------------
Ok, so we optimize away the lookup in the O_EXCL case (which is fine).
Then, we only attempt a posix open if O_CREAT was set (note that regular SMB CREATE_ANDX is prohibited here -- why?). Now, with Dan's patch, we'll be doing this only when O_CREAT is set, and only when the file actually exists on the server (since we'll be clearing
This really just makes no sense at all. The times when we can use this are so vanishingly small that it makes more sense to just remove this stuff. If you really want to do this, it needs a redesign from the ground up, with a consideration of each possible case and determination of whether it's better (and safe) to use an open instead of lookup in that case.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 56 lines · Jeff Layton
Quoted 44 lines · On Sat, 29 Oct 2011 00:07:12 -0500 Steve French <[email protected]> wrote:
Quoted 39 lines · Jeff Layton
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French <[email protected]> wrote:
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
Quoted 3 lines
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
Quoted 8 lines
We don't have much choice - we have to allow permission checking on the client when client security context can't match security context on server - but I would like to make multiuser the default (especially if we can figure out a way to integrate winbind-like upcall for ntlmv2 auth) at least for the future (smb2 etc.) even if we can't change the default for cifs mounts.
Quoted 3 lines
That's fine, but that doesn't solve Dan's immediate problem. We need to either take his patch or something like it for 3.2 (and probably for stable as well) or just rip this crap out altogether.
The benchmark gains are huge for a couple test cases
(e.g. creating lots of small files) - doesn't make sense to throw it out. At worst (I think we can do better) - we only give up on atomic open/create for when noperm is not set (we want our users
to be running noperm or equivalently multiuser). It is especially
important since the servers we do best at with create turn out to be Linux ( and other Unix which Samba runs on (due to posix extensions)).
If we break create apart as you suggest - we run into lookup/getattr/create/open/setattr
races which are also annoying - we are supposed to send an atomic create/open operation
to minimize strange consequences if the server file changes in between (the alternative)
where getattr/create/open/setattr is done.
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5muq4n34Liy_=CQQX-PeTinJFDAPi81fA-eqsTcXw5remQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 61 lines · On Sat, 29 Oct 2011 17:03:46 -0500 Steve French…
Quoted 56 lines · Jeff Layton
Quoted 44 lines · On Sat, 29 Oct 2011 00:07:12 -0500 Steve French…
Quoted 39 lines · Jeff Layton
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French…
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
Quoted 3 lines
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
Quoted 8 lines
We don't have much choice - we have to allow permission checking on the client when client security context can't match security context on server - but I would like to make multiuser the default (especially if we can figure out a way to integrate winbind-like upcall for ntlmv2 auth) at least for the future (smb2 etc.) even if we can't change the default for cifs mounts.
Quoted 3 lines
That's fine, but that doesn't solve Dan's immediate problem. We need to either take his patch or something like it for 3.2 (and probably for stable as well) or just rip this crap out altogether.
Quoted 8 lines
The benchmark gains are huge for a couple test cases
(e.g. creating lots of small files) - doesn't make sense to throw it out. At worst (I think we can do better) - we only give up on atomic open/create for when noperm is not set (we want our users
to be running noperm or equivalently multiuser). It is especially
important since the servers we do best at with create turn out to be Linux ( and other Unix which Samba runs on (due to posix extensions)).
I don't care at all about performance if the behavior is wrong. We need to worry about correctness first and only then optimize for performance. The create on lookup code has been the source of many regressions and I think it's time to pull the plug on it until someone wants to take the time to do it right.
I think it's obvious that the logic in cifs_lookup makes no sense. I'd prefer to see that ripped out until someone presents a design for this that does make sense, but if you want to present code that fixes this more logically then I'd be happy to look at it.
Quoted 8 lines
If we break create apart as you suggest - we run into lookup/getattr/create/open/setattr
races which are also annoying - we are supposed to send an atomic create/open operation
to minimize strange consequences if the server file changes in between (the alternative)
where getattr/create/open/setattr is done.
We only have to worry about atomicity in the O_EXCL case, and that is already covered in the earlier check in cifs_lookup.
Signature · 1 line
Jeff Layton <[email protected]>
A message that is not in the archive (sent off-list or not yet received): 20111029224852.15ee6302-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org
I thought we agreed that (at worst) the logic works for the "noperm" (no client side enforcement, ie multiuser case).
Quoted 99 lines · Jeff Layton
Quoted 61 lines · On Sat, 29 Oct 2011 17:03:46 -0500 Steve French…
Quoted 56 lines · Jeff Layton
Quoted 44 lines · On Sat, 29 Oct 2011 00:07:12 -0500 Steve French…
Quoted 39 lines · Jeff Layton
Quoted 22 lines · On Fri, 28 Oct 2011 10:27:04 -0500 Steve French…
Quoted 12 lines · Dan Carpenter
Quoted 11 lines · Steve French
Doesn't this force the create to happen later - rather than at lookup time where it belongs?
if the issue is just noperm ... we should let this through if the user has local permissions. Doubling the cost of a file create to Samba seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX doubles the roundrip, doubles the load on the server etc.) - the whole point of this is to let us do an "atomic" open or create operation and not have to split it into multiple requests. In any case why would we do the open and then follow it with a create?
Quoted 8 lines
Quoted 2 lines
Can we fix this to (at least) narrow the performance penalty.
Yes, it does add another back and forth to file create... It's hard/ impossible to check the permissions first and then decide whether to pass the
O_CREATflag. Maybe we could add an if statement to check whether noperm was used and the noperm version could use the create on lookup.Quoted 7 lines
Why is it hard to check local permissions? We have the local mode for the parent.
Also note that the "intent" flags and the atomic create is not just about performance, but also making it atomic reduces some of the weird failure cases.
We have the local mode for the parent, but we do not have the ownership and mode for the file that has not yet been created. Because of the special (ahem) way that cifs handles permissions, it's easily possible for the ownership of the file not to match the user doing that create. At that point, later operations on the file can easily fail.
Quoted 3 lines
This was the primary reason for the multiuser patch series, and why I still say that doing permissions checking on the client is a broken model.
Quoted 8 lines
We don't have much choice - we have to allow permission checking on the client when client security context can't match security context on server - but I would like to make multiuser the default (especially if we can figure out a way to integrate winbind-like upcall for ntlmv2 auth) at least for the future (smb2 etc.) even if we can't change the default for cifs mounts.
Quoted 3 lines
That's fine, but that doesn't solve Dan's immediate problem. We need to either take his patch or something like it for 3.2 (and probably for stable as well) or just rip this crap out altogether.
Quoted 8 lines
The benchmark gains are huge for a couple test cases
(e.g. creating lots of small files) - doesn't make sense to throw it out. At worst (I think we can do better) - we only give up on atomic open/create for when noperm is not set (we want our users
to be running noperm or equivalently multiuser). It is especially important since the servers we do best at with create turn out to be Linux ( and other Unix which Samba runs on (due to posix extensions)).I don't care at all about performance if the behavior is wrong. We need to worry about correctness first and only then optimize for performance. The create on lookup code has been the source of many regressions and I think it's time to pull the plug on it until someone wants to take the time to do it right.
I think it's obvious that the logic in
cifs_lookupmakes no sense. I'd prefer to see that ripped out until someone presents a design for this that does make sense, but if you want to present code that fixes this more logically then I'd be happy to look at it.Quoted 8 lines
If we break create apart as you suggest - we run into lookup/getattr/create/open/setattr
races which are also annoying - we are supposed to send an atomic create/open operation
to minimize strange consequences if the server file changes in between (the alternative)
where getattr/create/open/setattr is done.We only have to worry about atomicity in the
O_EXCLcase, and that is already covered in the earlier check incifs_lookup.--
Jeff Layton <[email protected]>
Signature · 3 lines
Thanks, Steve
A message that is not in the archive (sent off-list or not yet received): CAH2r5msVoEuxo8vGj6sYjjNqjPeo9wxa-r7bAq6Or1_xS9EWFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 3 lines · On Sat, 29 Oct 2011 21:53:13 -0500 Steve French…
I thought we agreed that (at worst) the logic works for the "noperm" (no client side enforcement, ie multiuser case).
It may work in that case, but that's really just an accident. It still doesn't make any sense for the reasons I've already described.
Signature · 1 line
Jeff Layton <[email protected]>