| Message ID | 20210203124112.1182614-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | new API for FS_IOC_[GS]ETFLAGS/FS_IOC_FS[GS]ETXATTR | expand |
On Wed, Feb 03, 2021 at 01:40:54PM +0100, Miklos Szeredi wrote: > This series adds the infrastructure and conversion of filesystems to the > new API. > > Two filesystems are not converted: FUSE and CIFS, as they behave > differently from local filesystems (use the file pointer, don't perform > permission checks). It's likely that these two can be supported with minor > changes to the API, but this requires more thought. Why not change the API now? ie pass the file instead of the dentry?
On Wed, Feb 3, 2021 at 2:08 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Feb 03, 2021 at 01:40:54PM +0100, Miklos Szeredi wrote: > > This series adds the infrastructure and conversion of filesystems to the > > new API. > > > > Two filesystems are not converted: FUSE and CIFS, as they behave > > differently from local filesystems (use the file pointer, don't perform > > permission checks). It's likely that these two can be supported with minor > > changes to the API, but this requires more thought. > > Why not change the API now? ie pass the file instead of the dentry? These are inode attributes we are talking about, not much sense in passing an open file to the filesystem. That was/is due to ioctl being an fd based API. It would make more sense to convert these filesystems to use a dentry instead of a file pointer. Which is not trivial, unfortuantely. Thanks, Miklos
On Wed, Feb 03, 2021 at 02:13:27PM +0100, Miklos Szeredi wrote: > On Wed, Feb 3, 2021 at 2:08 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Feb 03, 2021 at 01:40:54PM +0100, Miklos Szeredi wrote: > > > This series adds the infrastructure and conversion of filesystems to the > > > new API. > > > > > > Two filesystems are not converted: FUSE and CIFS, as they behave > > > differently from local filesystems (use the file pointer, don't perform > > > permission checks). It's likely that these two can be supported with minor > > > changes to the API, but this requires more thought. > > > > Why not change the API now? ie pass the file instead of the dentry? > > These are inode attributes we are talking about, not much sense in > passing an open file to the filesystem. That was/is due to ioctl > being an fd based API. You might as well say "Not much point passing a dentry to the filesystem" and just pass the inode. > It would make more sense to convert these filesystems to use a dentry > instead of a file pointer. Which is not trivial, unfortuantely. Network filesystems frequently need to use the credentials attached to a struct file in order to communicate with the server. There's no point fighting this reality.
On Wed, Feb 3, 2021 at 2:58 PM Matthew Wilcox <willy@infradead.org> wrote: > Network filesystems frequently need to use the credentials attached to > a struct file in order to communicate with the server. There's no point > fighting this reality. IDGI. Credentials can be taken from the file and from the task. In this case every filesystem except cifs looks at task creds. Why are network filesystem special in this respect? Thanks, Miklos
On Wed, Feb 03, 2021 at 03:13:29PM +0100, Miklos Szeredi wrote: > On Wed, Feb 3, 2021 at 2:58 PM Matthew Wilcox <willy@infradead.org> wrote: > > > Network filesystems frequently need to use the credentials attached to > > a struct file in order to communicate with the server. There's no point > > fighting this reality. > > IDGI. Credentials can be taken from the file and from the task. In > this case every filesystem except cifs looks at task creds. Why are > network filesystem special in this respect? I don't necessarily mean 'struct cred'. I mean "the authentication that the client has performed to the server". Which is not a per-task thing, it's stored in the struct file, which is why we have things like int (*write_begin)(struct file *, struct address_space *mapping, loff_t pos, unsigned len, unsigned flags, struct page **pagep, void **fsdata); disk filesystems ignore the struct file argument, but network filesystems very much use it.
On Wed, Feb 3, 2021 at 3:28 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Feb 03, 2021 at 03:13:29PM +0100, Miklos Szeredi wrote: > > On Wed, Feb 3, 2021 at 2:58 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > Network filesystems frequently need to use the credentials attached to > > > a struct file in order to communicate with the server. There's no point > > > fighting this reality. > > > > IDGI. Credentials can be taken from the file and from the task. In > > this case every filesystem except cifs looks at task creds. Why are > > network filesystem special in this respect? > > I don't necessarily mean 'struct cred'. I mean "the authentication > that the client has performed to the server". Which is not a per-task > thing, it's stored in the struct file, which is why we have things like > > int (*write_begin)(struct file *, struct address_space *mapping, > loff_t pos, unsigned len, unsigned flags, > struct page **pagep, void **fsdata); > > disk filesystems ignore the struct file argument, but network filesystems > very much use it. Fine for file I/O. That's authorized at open time for all filesystems, not just network ones. Not fine for metadata operations (IMO). I.e. ->[gs]etattr() don't take a file argument either, even though on the uAPI there are plenty of open file based variants. Thanks, Miklos
On Wed, Feb 03, 2021 at 03:38:54PM +0100, Miklos Szeredi wrote: > On Wed, Feb 3, 2021 at 3:28 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Feb 03, 2021 at 03:13:29PM +0100, Miklos Szeredi wrote: > > > On Wed, Feb 3, 2021 at 2:58 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > Network filesystems frequently need to use the credentials attached to > > > > a struct file in order to communicate with the server. There's no point > > > > fighting this reality. > > > > > > IDGI. Credentials can be taken from the file and from the task. In > > > this case every filesystem except cifs looks at task creds. Why are > > > network filesystem special in this respect? > > > > I don't necessarily mean 'struct cred'. I mean "the authentication > > that the client has performed to the server". Which is not a per-task > > thing, it's stored in the struct file, which is why we have things like > > > > int (*write_begin)(struct file *, struct address_space *mapping, > > loff_t pos, unsigned len, unsigned flags, > > struct page **pagep, void **fsdata); > > > > disk filesystems ignore the struct file argument, but network filesystems > > very much use it. > > Fine for file I/O. That's authorized at open time for all > filesystems, not just network ones. > > Not fine for metadata operations (IMO). I.e. ->[gs]etattr() don't > take a file argument either, even though on the uAPI there are plenty > of open file based variants. That's a fine statement of principle, but if the filesystem needs to contact the server, then your principle must accede to reality. But let's talk specifics. What does CIFS need to contact the server for? Could it be cached earlier?
On Wed, Feb 3, 2021 at 3:56 PM Matthew Wilcox <willy@infradead.org> wrote: > But let's talk specifics. What does CIFS need to contact the server for? > Could it be cached earlier? I don't understand what CIFS is doing, and I don't really care. This is the sort of operation where adding a couple of network roundtrips so that the client can obtain the credentials required to perform the operation doesn't really matter. We won't have thousands of chattr(1) calls per second. So I think the principle is more important than the details of the current implementation. And I'm saying that knowing that fixing up FUSE will be my responsibility and it won't be trivial either. Thanks, Miklos
On Wed, Feb 03, 2021 at 04:03:06PM +0100, Miklos Szeredi wrote: > On Wed, Feb 3, 2021 at 3:56 PM Matthew Wilcox <willy@infradead.org> wrote: > > > But let's talk specifics. What does CIFS need to contact the server for? > > Could it be cached earlier? > > I don't understand what CIFS is doing, and I don't really care. This > is the sort of operation where adding a couple of network roundtrips > so that the client can obtain the credentials required to perform the > operation doesn't really matter. We won't have thousands of chattr(1) > calls per second. Incorrect. The xfs utilities can do recursive directory traversal to change things like the project ID across an entire directory tree. Or to change extent size hints. We also have 'xfs_io -c "lsattr -R" ...' and 'lsprog -R' which will do a recursive descent to list the requested attributes of all directories and files in the tree... So, yeah, we do indeed do thousands of these fsxattr based operations a second, sometimes tens of thousands a second or more, and sometimes they are issued in bulk in performance critical paths for container build/deployment operations.... Cheers, Dave.
On Mon, Feb 8, 2021 at 3:00 AM Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Feb 03, 2021 at 04:03:06PM +0100, Miklos Szeredi wrote: > > On Wed, Feb 3, 2021 at 3:56 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > But let's talk specifics. What does CIFS need to contact the server for? > > > Could it be cached earlier? > > > > I don't understand what CIFS is doing, and I don't really care. This > > is the sort of operation where adding a couple of network roundtrips > > so that the client can obtain the credentials required to perform the > > operation doesn't really matter. We won't have thousands of chattr(1) > > calls per second. > > Incorrect. Okay, I was wrong. Still, CIFS may very well be able to perform these operations without a struct file. But even if it can't, I'd still only add the file pointer as an *optional hint* from the VFS, not as the primary object as Matthew suggested. I stand by my choice of /struct dentry/ as the object to pass to these operations. Thanks, Miklos
On Mon, Feb 08, 2021 at 09:25:22AM +0100, Miklos Szeredi wrote: > On Mon, Feb 8, 2021 at 3:00 AM Dave Chinner <david@fromorbit.com> wrote: > > > > On Wed, Feb 03, 2021 at 04:03:06PM +0100, Miklos Szeredi wrote: > > > On Wed, Feb 3, 2021 at 3:56 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > But let's talk specifics. What does CIFS need to contact the server for? > > > > Could it be cached earlier? > > > > > > I don't understand what CIFS is doing, and I don't really care. This > > > is the sort of operation where adding a couple of network roundtrips > > > so that the client can obtain the credentials required to perform the > > > operation doesn't really matter. We won't have thousands of chattr(1) > > > calls per second. > > > > Incorrect. > > Okay, I was wrong. > > Still, CIFS may very well be able to perform these operations without > a struct file. But even if it can't, I'd still only add the file > pointer as an *optional hint* from the VFS, not as the primary object > as Matthew suggested. > > I stand by my choice of /struct dentry/ as the object to pass to these > operations. Why the dentry? This is an inode operation. Why doesn't it take an inode as its primary object?
On Mon, Feb 8, 2021 at 3:02 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Feb 08, 2021 at 09:25:22AM +0100, Miklos Szeredi wrote: > > On Mon, Feb 8, 2021 at 3:00 AM Dave Chinner <david@fromorbit.com> wrote: > > > > > > On Wed, Feb 03, 2021 at 04:03:06PM +0100, Miklos Szeredi wrote: > > > > On Wed, Feb 3, 2021 at 3:56 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > But let's talk specifics. What does CIFS need to contact the server for? > > > > > Could it be cached earlier? > > > > > > > > I don't understand what CIFS is doing, and I don't really care. This > > > > is the sort of operation where adding a couple of network roundtrips > > > > so that the client can obtain the credentials required to perform the > > > > operation doesn't really matter. We won't have thousands of chattr(1) > > > > calls per second. > > > > > > Incorrect. > > > > Okay, I was wrong. > > > > Still, CIFS may very well be able to perform these operations without > > a struct file. But even if it can't, I'd still only add the file > > pointer as an *optional hint* from the VFS, not as the primary object > > as Matthew suggested. > > > > I stand by my choice of /struct dentry/ as the object to pass to these > > operations. > > Why the dentry? This is an inode operation. Why doesn't it take an > inode as its primary object? If we pass struct file to the op, then that limits callers to those that have an open file. E.g. it would be difficult to introduce a path based userspace API for getting/changing these attributes. Passing a dentry instead of an inode has no such problem, since the dentry is always available, whether coming from an open file or a path. Some inode operations do pass a dentry instead of an inode (readlink, getattr, setattr) and some filesystems take advantage of this. Thanks, Miklos