Messages in this thread | | | Date | Wed, 11 May 2022 15:30:09 -0400 | From | Vivek Goyal <> | Subject | Re: [PATCH v4 0/3] FUSE: Implement atomic lookup + open/create |
| |
On Wed, May 11, 2022 at 01:21:13PM -0400, Vivek Goyal wrote: > On Wed, May 11, 2022 at 11:40:59AM +0200, Miklos Szeredi wrote: > > On Thu, 5 May 2022 at 21:59, Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > Oh, I have no issues with the intent. I will like to see cut in network > > > traffic too (if we can do this without introducing problems). My primary > > > interest is that this kind of change should benefit virtiofs as well. > > > > One issue with that appears to be checking permissions. AFAIU this > > patchset only enables the optimization if default_permissions is > > turned off (i.e. all permission checking is done by the server). But > > virtiofs uses the default_permissions model. > > IIUC, only 3rd patch mentions that default_permission should be turned > off. IOW, first patch where lookup + create + open is a single operation > and second patch which does "lookup + open" in a single operation does > not seem to require that default_permissions are not in effect. > > So if first two patches work fine, I think virtiofs should benefit too. > (IMHO, 3rd patch is too hacky anyway) > > W.r.t permission checks, looks like may_open() will finally be called > after ->atomic_open(). So even if we open the file, we should still be > able to check whether we have permissions to open the file or not > after the fact. > > fs/namei.c > > path_openat() > { > open_last_lookups() <--- This calls ->atomic_open() > do_open() <--- This calls may_open() > }
Actually I am not sure about it. I was playing with
open(foo.txt, O_CREAT | O_RDWR, O_IRUSR)
This succeeds if file is newly created but if file already existed, this fails with -EACCESS
So man 2 open says following. Thanks to Andy Price for pointing me to it.
Note that mode applies only to future accesses of the newly cre‐ ated file; the open() call that creates a read-only file may well return a read/write file descriptor.
Now I am wondering how will it look like with first patch. Assume file already exists on the server (But there is no negative dentry present) and I do following. And assume file only has read permission for user and I am trying to open it read-write.
open(foo.txt, O_CREAT | O_RDWR, O_IRUSR)
In normal circumstances, user will expect -EACCESS as file is read-only and user is trying to open it read-write.
I am wondering how will it look like with this first patch.
Current fuse ->atomic_open() looks up the dentry and does not open the file if dentry is positive.
New implementation will skip lookup and open the file anyway and set file->f_mode |= FMODE_CREATED; (First patch in series)
So first of all this seems wrong. I thought FMODE_CREATED should be set only if file was newly created. Is that a correct understanding.
And I am looking at do_open() code. It does bunch of things based on FMODE_CREATED flag. One of the things it does is reset acc_mode =0
if (file->f_mode & FMODE_CREATED) { /* Don't check for write permission, don't truncate */ open_flag &= ~O_TRUNC; acc_mode = 0; } error = may_open(mnt_userns, &nd->path, acc_mode, open_flag);
I suspect this is the code which allows opening a newly created read-only file as O_RDWR. (Though I am not 100% sure).
I suspect with first patch this will be broken. We will set FMODE_CREATED even if file already existed and VFS will assume a new file has been created and do bunch of things which is wrong.
So looks like fuse ->atomic_open() should set FMODE_CREATED only if it really created the file.
Thanks Vivek
| |