| Message ID | 151297224512.7818.18333908109878259066.stgit@noble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 11, 2017 at 05:04:05PM +1100, NeilBrown wrote: > @@ -385,7 +385,9 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, > { > const struct export_operations *nop = inode->i_sb->s_export_op; > > - if (nop && nop->encode_fh) > + if (nop) > + return FILEID_INVALID; > + if (nop->encode_fh) > return nop->encode_fh(inode, fid->raw, max_len, parent); This might as well have been if (nop) return FILEID_INVALID; BUG(); Have you ever tested that?
On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: > If a filesystem does not set sb->s_export_op, then it > does not support filehandles and export_fs_encode_fh() > and exportfs_encode_inode_fh() should not be called. > They will use export_encode_fh() is which is a default > that uses inode number generation number, but in general > they may not be stable. > > So change exportfs_encode_inode_fh() to return FILEID_INVALID > if called on an unsupported Filesystem. Currently only > notify/fdinfo can do that. > I wish you would leave this check to the caller, maybe add a helper exportfs_can_decode_fh() for callers to use. Although there are no current uses for it in-tree, there is value in being able to encode a unique file handle even when it cannot be decoded back to an open file. I am using this property in my fanotify super block watch patches, where the object identifier on the event is an encoded file handle of the object, which delegates tracking filesystem objects to userspace and prevents fanotify from keeping elevated refcounts on inodes and dentries. There are quite a few userspace tools out there that are checking that st_ino hasn't changed on a file between non atomic operations. Those tools (or others) could benefit from a unique file handle if we ever decide to provide a relaxed version of name_to_handle_at(). Cheers, Amir.
On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote: > On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >> If a filesystem does not set sb->s_export_op, then it >> does not support filehandles and export_fs_encode_fh() >> and exportfs_encode_inode_fh() should not be called. >> They will use export_encode_fh() is which is a default >> that uses inode number generation number, but in general >> they may not be stable. >> >> So change exportfs_encode_inode_fh() to return FILEID_INVALID >> if called on an unsupported Filesystem. Currently only >> notify/fdinfo can do that. >> > > I wish you would leave this check to the caller, maybe add a helper > exportfs_can_decode_fh() for callers to use. > > Although there are no current uses for it in-tree, there is value in > being able to encode a unique file handle even when it cannot be > decoded back to an open file. > > I am using this property in my fanotify super block watch patches, > where the object identifier on the event is an encoded file handle > of the object, which delegates tracking filesystem objects to > userspace and prevents fanotify from keeping elevated refcounts > on inodes and dentries. > > There are quite a few userspace tools out there that are checking > that st_ino hasn't changed on a file between non atomic operations. > Those tools (or others) could benefit from a unique file handle if > we ever decide to provide a relaxed version of name_to_handle_at(). > And this change need a clause about not breaking userspace. Pavel, Will this break any version of criu in the wild? Amir.
On 12/11/2017 10:05 AM, Amir Goldstein wrote: > On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >>> If a filesystem does not set sb->s_export_op, then it >>> does not support filehandles and export_fs_encode_fh() >>> and exportfs_encode_inode_fh() should not be called. >>> They will use export_encode_fh() is which is a default >>> that uses inode number generation number, but in general >>> they may not be stable. >>> >>> So change exportfs_encode_inode_fh() to return FILEID_INVALID >>> if called on an unsupported Filesystem. Currently only >>> notify/fdinfo can do that. >>> >> >> I wish you would leave this check to the caller, maybe add a helper >> exportfs_can_decode_fh() for callers to use. >> >> Although there are no current uses for it in-tree, there is value in >> being able to encode a unique file handle even when it cannot be >> decoded back to an open file. >> >> I am using this property in my fanotify super block watch patches, >> where the object identifier on the event is an encoded file handle >> of the object, which delegates tracking filesystem objects to >> userspace and prevents fanotify from keeping elevated refcounts >> on inodes and dentries. >> >> There are quite a few userspace tools out there that are checking >> that st_ino hasn't changed on a file between non atomic operations. >> Those tools (or others) could benefit from a unique file handle if >> we ever decide to provide a relaxed version of name_to_handle_at(). >> > > And this change need a clause about not breaking userspace. > > Pavel, > > Will this break any version of criu in the wild? If there's no fliehandle in the output, it will make dump fail, but we're already prepared for the fact, that there's no handle at hands. In the worst case criu will exit with error. I also agree that it should only happen when current is OOM killed, and in case of CRIU this means killing criu process itself. -- Pavel
On Mon, Dec 11, 2017 at 3:46 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote: > On 12/11/2017 10:05 AM, Amir Goldstein wrote: >> On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote: >>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >>>> If a filesystem does not set sb->s_export_op, then it >>>> does not support filehandles and export_fs_encode_fh() >>>> and exportfs_encode_inode_fh() should not be called. >>>> They will use export_encode_fh() is which is a default >>>> that uses inode number generation number, but in general >>>> they may not be stable. >>>> >>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID >>>> if called on an unsupported Filesystem. Currently only >>>> notify/fdinfo can do that. >>>> >>> >>> I wish you would leave this check to the caller, maybe add a helper >>> exportfs_can_decode_fh() for callers to use. >>> >>> Although there are no current uses for it in-tree, there is value in >>> being able to encode a unique file handle even when it cannot be >>> decoded back to an open file. >>> >>> I am using this property in my fanotify super block watch patches, >>> where the object identifier on the event is an encoded file handle >>> of the object, which delegates tracking filesystem objects to >>> userspace and prevents fanotify from keeping elevated refcounts >>> on inodes and dentries. >>> >>> There are quite a few userspace tools out there that are checking >>> that st_ino hasn't changed on a file between non atomic operations. >>> Those tools (or others) could benefit from a unique file handle if >>> we ever decide to provide a relaxed version of name_to_handle_at(). >>> >> >> And this change need a clause about not breaking userspace. >> >> Pavel, >> >> Will this break any version of criu in the wild? > > If there's no fliehandle in the output, it will make dump fail, but we're > already prepared for the fact, that there's no handle at hands. In the > worst case criu will exit with error. > > I also agree that it should only happen when current is OOM killed, and in > case of CRIU this means killing criu process itself. > But this patch [1/4] changes behavior so you cannot dump fsnotify state if watched file system does not support *decoding* file handles. This means that criu anyway won't be able to restore the fsnotify state. Is it OK that criu dump state will fail in that case? Amir.
On 12/11/2017 05:08 PM, Amir Goldstein wrote: > On Mon, Dec 11, 2017 at 3:46 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote: >> On 12/11/2017 10:05 AM, Amir Goldstein wrote: >>> On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >>>>> If a filesystem does not set sb->s_export_op, then it >>>>> does not support filehandles and export_fs_encode_fh() >>>>> and exportfs_encode_inode_fh() should not be called. >>>>> They will use export_encode_fh() is which is a default >>>>> that uses inode number generation number, but in general >>>>> they may not be stable. >>>>> >>>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID >>>>> if called on an unsupported Filesystem. Currently only >>>>> notify/fdinfo can do that. >>>>> >>>> >>>> I wish you would leave this check to the caller, maybe add a helper >>>> exportfs_can_decode_fh() for callers to use. >>>> >>>> Although there are no current uses for it in-tree, there is value in >>>> being able to encode a unique file handle even when it cannot be >>>> decoded back to an open file. >>>> >>>> I am using this property in my fanotify super block watch patches, >>>> where the object identifier on the event is an encoded file handle >>>> of the object, which delegates tracking filesystem objects to >>>> userspace and prevents fanotify from keeping elevated refcounts >>>> on inodes and dentries. >>>> >>>> There are quite a few userspace tools out there that are checking >>>> that st_ino hasn't changed on a file between non atomic operations. >>>> Those tools (or others) could benefit from a unique file handle if >>>> we ever decide to provide a relaxed version of name_to_handle_at(). >>>> >>> >>> And this change need a clause about not breaking userspace. >>> >>> Pavel, >>> >>> Will this break any version of criu in the wild? >> >> If there's no fliehandle in the output, it will make dump fail, but we're >> already prepared for the fact, that there's no handle at hands. In the >> worst case criu will exit with error. >> >> I also agree that it should only happen when current is OOM killed, and in >> case of CRIU this means killing criu process itself. >> > > But this patch [1/4] changes behavior so you cannot dump fsnotify > state if watched file system does not support *decoding* file handles. That's OK :) After we get a filehandle we check it's accessible, so for FSs that couldn't decode one, we'd fail the dump anyway. > This means that criu anyway won't be able to restore the fsnotify state. > Is it OK that criu dump state will fail in that case? > > Amir. > . >
On Mon, Dec 11 2017, Amir Goldstein wrote: > On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >> If a filesystem does not set sb->s_export_op, then it >> does not support filehandles and export_fs_encode_fh() >> and exportfs_encode_inode_fh() should not be called. >> They will use export_encode_fh() is which is a default >> that uses inode number generation number, but in general >> they may not be stable. >> >> So change exportfs_encode_inode_fh() to return FILEID_INVALID >> if called on an unsupported Filesystem. Currently only >> notify/fdinfo can do that. >> > > I wish you would leave this check to the caller, maybe add a helper > exportfs_can_decode_fh() for callers to use. > > Although there are no current uses for it in-tree, there is value in > being able to encode a unique file handle even when it cannot be > decoded back to an open file. > > I am using this property in my fanotify super block watch patches, > where the object identifier on the event is an encoded file handle > of the object, which delegates tracking filesystem objects to > userspace and prevents fanotify from keeping elevated refcounts > on inodes and dentries. > > There are quite a few userspace tools out there that are checking > that st_ino hasn't changed on a file between non atomic operations. > Those tools (or others) could benefit from a unique file handle if > we ever decide to provide a relaxed version of name_to_handle_at(). If the filesystem doesn't define ->s_export_op, then you really cannot trust anything beyond the inode number (and maybe not even that), and the inode number is already easily available. What actual value do you think you get from this pretend-file-handle on filesystems that don't support file handles? If there is a demonstrated need for some sort of identifier that is stronger than an inode number, but not strong enough for open_by_handle_at(), then you should explain that need and propose a well defined interface. You shouldn't use a back-door and hope no-one notices. Thanks, NeilBrown
On Mon, Dec 11, 2017 at 11:52 PM, NeilBrown <neilb@suse.com> wrote: > On Mon, Dec 11 2017, Amir Goldstein wrote: > >> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >>> If a filesystem does not set sb->s_export_op, then it >>> does not support filehandles and export_fs_encode_fh() >>> and exportfs_encode_inode_fh() should not be called. >>> They will use export_encode_fh() is which is a default >>> that uses inode number generation number, but in general >>> they may not be stable. >>> >>> So change exportfs_encode_inode_fh() to return FILEID_INVALID >>> if called on an unsupported Filesystem. Currently only >>> notify/fdinfo can do that. >>> >> >> I wish you would leave this check to the caller, maybe add a helper >> exportfs_can_decode_fh() for callers to use. >> >> Although there are no current uses for it in-tree, there is value in >> being able to encode a unique file handle even when it cannot be >> decoded back to an open file. >> >> I am using this property in my fanotify super block watch patches, >> where the object identifier on the event is an encoded file handle >> of the object, which delegates tracking filesystem objects to >> userspace and prevents fanotify from keeping elevated refcounts >> on inodes and dentries. >> >> There are quite a few userspace tools out there that are checking >> that st_ino hasn't changed on a file between non atomic operations. >> Those tools (or others) could benefit from a unique file handle if >> we ever decide to provide a relaxed version of name_to_handle_at(). > > If the filesystem doesn't define ->s_export_op, then you really cannot > trust anything beyond the inode number (and maybe not even that), and > the inode number is already easily available. > What actual value do you think you get from this pretend-file-handle > on filesystems that don't support file handles? > Sorry, I misread your patch. In my mind I thought you wanted to eliminate the default export_encode_fh if there was no fh_to_dentry operation like do_sys_name_to_handle() does. Just in my head... FWIW, according to Pavel, if fdinfo would not export file handle in case !fh_to_dentry op would probably be desirable, because criu has no need for file handles that cannot be decoded. Amir.
On Tue, Dec 12 2017, Amir Goldstein wrote: > On Mon, Dec 11, 2017 at 11:52 PM, NeilBrown <neilb@suse.com> wrote: >> On Mon, Dec 11 2017, Amir Goldstein wrote: >> >>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote: >>>> If a filesystem does not set sb->s_export_op, then it >>>> does not support filehandles and export_fs_encode_fh() >>>> and exportfs_encode_inode_fh() should not be called. >>>> They will use export_encode_fh() is which is a default >>>> that uses inode number generation number, but in general >>>> they may not be stable. >>>> >>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID >>>> if called on an unsupported Filesystem. Currently only >>>> notify/fdinfo can do that. >>>> >>> >>> I wish you would leave this check to the caller, maybe add a helper >>> exportfs_can_decode_fh() for callers to use. >>> >>> Although there are no current uses for it in-tree, there is value in >>> being able to encode a unique file handle even when it cannot be >>> decoded back to an open file. >>> >>> I am using this property in my fanotify super block watch patches, >>> where the object identifier on the event is an encoded file handle >>> of the object, which delegates tracking filesystem objects to >>> userspace and prevents fanotify from keeping elevated refcounts >>> on inodes and dentries. >>> >>> There are quite a few userspace tools out there that are checking >>> that st_ino hasn't changed on a file between non atomic operations. >>> Those tools (or others) could benefit from a unique file handle if >>> we ever decide to provide a relaxed version of name_to_handle_at(). >> >> If the filesystem doesn't define ->s_export_op, then you really cannot >> trust anything beyond the inode number (and maybe not even that), and >> the inode number is already easily available. >> What actual value do you think you get from this pretend-file-handle >> on filesystems that don't support file handles? >> > > Sorry, I misread your patch. In my mind I thought you wanted to > eliminate the default export_encode_fh if there was no fh_to_dentry > operation like do_sys_name_to_handle() does. Just in my head... I see... I would have said that fh_to_dentry was mandatory if s_export_op was set, and Documentation/filesystems/nfs/Exporting agrees with me. But I do see that sys_name_to_handle() and nfsd explicitly test for it as well as for s_export_op. It appears that cifs sets s_export_op, but doesn't provide fh_to_dentry, and it is unique in this. But the CIFS_NFSD_EXPORT config option is marked as BROKEN, so that probably doesn't matter. So for all current filesystems, filehandles are only reported if they are usable for dentry lookup... > > FWIW, according to Pavel, if fdinfo would not export file handle > in case !fh_to_dentry op would probably be desirable, because > criu has no need for file handles that cannot be decoded. Yes, it was good to have that confirmed - thanks for getting that checked. NeilBrown
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c index 329a5d103846..f5b27dd843a1 100644 --- a/fs/exportfs/expfs.c +++ b/fs/exportfs/expfs.c @@ -385,7 +385,9 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, { const struct export_operations *nop = inode->i_sb->s_export_op; - if (nop && nop->encode_fh) + if (nop) + return FILEID_INVALID; + if (nop->encode_fh) return nop->encode_fh(inode, fid->raw, max_len, parent); return export_encode_fh(inode, fid, max_len, parent); diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c index d478629c728b..d1135ed61229 100644 --- a/fs/notify/fdinfo.c +++ b/fs/notify/fdinfo.c @@ -50,10 +50,8 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode) size = f.handle.handle_bytes >> 2; ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, 0); - if ((ret == FILEID_INVALID) || (ret < 0)) { - WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret); + if ((ret == FILEID_INVALID) || (ret < 0)) return; - } f.handle.handle_type = ret; f.handle.handle_bytes = size * sizeof(u32);
If a filesystem does not set sb->s_export_op, then it does not support filehandles and export_fs_encode_fh() and exportfs_encode_inode_fh() should not be called. They will use export_encode_fh() is which is a default that uses inode number generation number, but in general they may not be stable. So change exportfs_encode_inode_fh() to return FILEID_INVALID if called on an unsupported Filesystem. Currently only notify/fdinfo can do that. Also remove the WARNing from fdinfo when exportfs_encode_inode_fh() returns FILEID_INVALID, as that is no an erroneous condition. Signed-off-by: NeilBrown <neilb@suse.com> --- fs/exportfs/expfs.c | 4 +++- fs/notify/fdinfo.c | 4 +--- 2 files changed, 4 insertions(+), 4 deletions(-)