# Re: \[PATCH v3 1/5\] add metadata\_incore ioctl in vfs - List: linux-api - 4 messages, 4 authors - Page: https://ratatoskr.run/linux-api/2011/01/50432/t ## Re: \[PATCH v3 1/5\] add metadata\_incore ioctl in vfs - From: Shaohua Li \ - Date: 2011-01-20 02:30:47 - Message: https://ratatoskr.run/linux-api/2011/01/50432.md ```text On Thu, 2011-01-20 at 04:41 +0800, Andrew Morton wrote: > On Wed, 19 Jan 2011 09:15:18 +0800 > Shaohua Li [ref] wrote: > > > Subject: add metadata_incore ioctl in vfs > > > > Add an ioctl to dump filesystem's metadata in memory in vfs. Userspace collects > > such info and uses it to do metadata readahead. > > Filesystem can hook to super_operations.metadata_incore to get metadata in > > specific approach. Next patch will give an example how to implement > > .metadata_incore in btrfs. > > > > ... > > > > /* > > + * Copy info about metadata in memory to userspace > > + * Returns: > > + * = 1, one metadata range copied to userspace > > + * = 0, no more metadata > > + * < 0, error > > + */ > > +static int ioctl_metadata_incore(struct file *filp, void __user *argp) > > +{ > > + struct super_block *sb = filp->f_path.dentry->d_inode->i_sb; > > + struct metadata_incore_args args; > > + loff_t offset; > > + ssize_t size; > > + > > + if (!sb->s_op->metadata_incore) > > + return -EINVAL; > > + > > + if (copy_from_user(&args, argp, sizeof(args))) > > + return -EFAULT; > > + > > + /* we check metadata info in page unit */ > > + if (args.offset & ~PAGE_CACHE_MASK) > > + return -EINVAL; > > + > > + offset = args.offset; > > + > > + if (sb->s_op->metadata_incore(sb, &offset, &size) < 0) > > + return 0; > > + > > + args.address = offset; > > + args.size = size; > > + args.unused = 0; > > + > > + if (copy_to_user(argp, &args, sizeof(args))) > > + return -EFAULT; > > + > > + return 1; > > +} > > So userspace opens any file on the fs and runs this ioctl against it? > > That's a pretty awkward interface - we're doing an fs-wide operation > but the fs is identified by a single file which happens to live on that > fs. For example, this precludes a future extension whereby userspace > can query the incore metadata for a particular file. The statfs > syscall sucks in the same manner. > > I don't know if this is worth addressing. Perhaps require that the > filp refers to the root of the fs? I didn't see why this is needed, but I can limit the fip to the root of the fs. > Also, is this a privileged operation? If not, then that might be a > problem - could it be used by unprivileged users to work out which > files have been opened recently or something like that? it's harmless even a unprivileged user uses it. I don't think unprivileged user can decode the data returned from the ioctl. Thanks, Shaohua ``` ## Re: \[PATCH v3 1/5\] add metadata\_incore ioctl in vfs - From: Andrew Morton \ - Date: 2011-01-20 02:42:40 - Message: https://ratatoskr.run/linux-api/2011/01/50434.md ```text On Thu, 20 Jan 2011 10:30:47 +0800 Shaohua Li [ref] wrote: > > I don't know if this is worth addressing. Perhaps require that the > > filp refers to the root of the fs? > I didn't see why this is needed, but I can limit the fip to the root of > the fs. I don't think it matters much either. The only problem I can see is if we were to later try to extend the ioctl into a per-file thing. > > Also, is this a privileged operation? If not, then that might be a > > problem - could it be used by unprivileged users to work out which > > files have been opened recently or something like that? > it's harmless even a unprivileged user uses it. I don't think > unprivileged user can decode the data returned from the ioctl. um. Well, by doing a before-and-after thing I can use this ioctl to work out what metadata blocks are used when someone reads /my/super/secret-directory/foo. Then I can write a program which sits there waiting until someone else reads /my/super/secret-directory/foo. Then I can use that information to start WWIII or something. I dunno, strange things happen. Unless there's a good *need* to make this available to unprivileged users then we should not do so. ``` ## Re: \[PATCH v3 1/5\] add metadata\_incore ioctl in vfs - From: Shaohua Li \ - Date: 2011-01-20 02:48:57 - Message: https://ratatoskr.run/linux-api/2011/01/50436.md ```text On Thu, 2011-01-20 at 10:42 +0800, Andrew Morton wrote: > On Thu, 20 Jan 2011 10:30:47 +0800 Shaohua Li [ref] wrote: > > > > I don't know if this is worth addressing. Perhaps require that the > > > filp refers to the root of the fs? > > I didn't see why this is needed, but I can limit the fip to the root of > > the fs. > > I don't think it matters much either. The only problem I can see is if > we were to later try to extend the ioctl into a per-file thing. since we return page range, a metadata page might be shared by several files, which makes the per-file thing doesn't work. For a fs using trees, it's even more hard to distinguish a file's metadata > > > Also, is this a privileged operation? If not, then that might be a > > > problem - could it be used by unprivileged users to work out which > > > files have been opened recently or something like that? > > it's harmless even a unprivileged user uses it. I don't think > > unprivileged user can decode the data returned from the ioctl. > > um. > > Well, by doing a before-and-after thing I can use this ioctl to work > out what metadata blocks are used when someone reads > /my/super/secret-directory/foo. Then I can write a program which sits > there waiting until someone else reads /my/super/secret-directory/foo. > Then I can use that information to start WWIII or something. > > I dunno, strange things happen. Unless there's a good *need* to make > this available to unprivileged users then we should not do so. ok, looks interesting, I'll update the patch to limit unprivileged users. Thanks, Shaohua ``` ## Re: \[PATCH v3 1/5\] add metadata\_incore ioctl in vfs - From: Andrew Morton \ - Date: 2011-01-20 03:05:48 - Message: https://ratatoskr.run/linux-api/2011/01/50438.md ```text On Thu, 20 Jan 2011 10:48:33 +0800 Shaohua Li [ref] wrote: > On Thu, 2011-01-20 at 10:42 +0800, Andrew Morton wrote: > > On Thu, 20 Jan 2011 10:30:47 +0800 Shaohua Li [ref] wrote: > > > > > > I don't know if this is worth addressing. Perhaps require that the > > > > filp refers to the root of the fs? > > > I didn't see why this is needed, but I can limit the fip to the root of > > > the fs. > > > > I don't think it matters much either. The only problem I can see is if > > we were to later try to extend the ioctl into a per-file thing. > since we return page range, a metadata page might be shared by several > files, which makes the per-file thing doesn't work. For a fs using > trees, it's even more hard to distinguish a file's metadata hm, why. A query for "which blocks need to be read to access this file" may return blocks which are shared with other files, but it's still useful info. Because it will represent vastly less data (and hence IO) than the current fs-wide thing. Now I actually look at it, I cannot find any documentation for the ioctl! It seems to return a single offset/length tuple which refers to the btrfs metadata "file", with the intent that this tuple later be fed into a btrfs-specific readahead ioctl. I can see how this might be used with say fatfs or ext3 where all metadata resides within the blockdev address_space. But how is a filesytem which keeps its metadata in multiple address_spaces supposed to use this interface? So. Please fully document the proposed userspace APIs! This should be the first thing we look at. Then we can take a look at how applicable that is to other-than-btrfs filesystems. ```