Providing additional information in fdinfo sufficient for c/r

This patch brings ability to print out auxiliary data associated with file in procfs interface /proc/pid/fdinfo/fd.

In particular further patches make eventfd, evenpoll, signalfd and fsnotify to print additional information complete enough to restore these objects after checkpoint.

To simplify the code we add show_fdinfo callback inside struct file_operations (as Al and Pavel are proposing).

...

Quoted 7 lines

include/linux/fs.h +0 -0

@@ -1517,6 +1517,8 @@ struct block_device_operations;
 #define HAVE_COMPAT_IOCTL 1
 #define HAVE_UNLOCKED_IOCTL 1
 

include/linux/fs.h struct block_device_operations; · line 1521

Quoted 2 lines

include/linux/fs.h +2 -0

+struct seq_file;
+

We already have a forward declaration of seq_file in fs.h. At line 1583(!).

This is why forward declarations should always be placed at the start of the file, please.

This file handle will be used in /proc/pid/fdinfo/fd output, which in turn will allow to restore a watch target after checkpoint (thus it's provided for CONFIG_CHECKPOINT_RESTORE only).

fs/notify/inotify/inotify.h +1 -0

@@ -1,6 +1,7 @@
 #include <linux/fsnotify_backend.h>
 #include <linux/inotify.h>
 #include <linux/slab.h> /* struct kmem_cache */
+#include <linux/exportfs.h>
 
 extern struct kmem_cache *event_priv_cachep;
 

fs/notify/inotify/inotify.h +0 -0

 extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,

Index: linux-2.6.git/fs/notify/inotify/inotify_user.c
===================================================================

fs/notify/inotify/inotify_user.c +3 -0

@@ -566,6 +566,33 @@ static void inotify_free_mark(struct fsn
 	kmem_cache_free(inotify_inode_mark_cachep, i_mark);
 }
 
+#ifdef INOTIFY_USE_FHANDLE
+static int inotify_encode_wd_fhandle(struct inotify_inode_mark *mark, struct dentry *dentry)
+{

fs/notify/inotify/inotify_user.c +5 -0

+	fhandle->handle_type = ret;
+	fhandle->handle_bytes = size * sizeof(u32);
+
+	return 0;
+}
 
...

Quoted 4 lines · On Mon, 12 Nov 2012 14:14:43 +0400 Cyrill Gorcunov <[email protected]> wrote:

This file handle will be used in /proc/pid/fdinfo/fd output, which in turn will allow to restore a watch target after checkpoint (thus it's provided for CONFIG_CHECKPOINT_RESTORE only).

This changelog isn't very good.

What appears to be happening is that you're borrowing exportfs's ability to encode file handles and this is being reused to transport inotify fd's to userspace for c/r? Or something else - I didn't try very hard. Please explain better?

Quoted 14 lines

fs/notify/inotify/inotify.h +1 -0

@@ -1,6 +1,7 @@
 #include <linux/fsnotify_backend.h>
 #include <linux/inotify.h>
 #include <linux/slab.h> /* struct kmem_cache */
+#include <linux/exportfs.h>
 
 extern struct kmem_cache *event_priv_cachep;
 

@@ -9,9 +10,16 @@ struct inotify_event_private_data {
 	int wd;
 };
 

fs/notify/inotify/inotify.h struct inotify_event_private_data { · line 15

Quoted 3 lines

fs/notify/inotify/inotify.h +3 -0

+#if defined(CONFIG_PROC_FS) && defined(CONFIG_EXPORTFS) && defined(CONFIG_CHECKPOINT_RESTORE)
+# define INOTIFY_USE_FHANDLE
+#endif

Does this mean that c/r will fail to work properly if CONFIG_EXPORTFS=n? If so, that should be expressed in Kconfig dependencies?

fs/notify/inotify/inotify.h struct inotify_event_private_data { · line 23

Quoted 7 lines

fs/notify/inotify/inotify.h +3 -0

 struct inotify_inode_mark {
 	struct fsnotify_mark fsn_mark;
 	int wd;
+#ifdef INOTIFY_USE_FHANDLE
+	__u8 fhandle[sizeof(struct file_handle) + MAX_HANDLE_SZ];
+#endif
 };

Whoa. This adds 128+8 bytes to the inotify_inode_mark. That's a lot of bloat, and there can be a lot of inotify_inode_mark's in the system?

Also, what about alignment? That embedded `struct file_handle' will have a 4-byte alignment and if there's anything in there which is an 8-byte quantity then some architectures (ia64?) might get upset about the kernel-mode unaligned access. It appears that this won't happen in the present code, but are we future-proof?

Why did you use a __u8, anyway? Could have done something like

	struct {
		struct file_handle fhandle;
		u8 pad[MAX_HANDLE_SZ];
	};

and got some additional type safety and less typecasting?

Quoted 9 lines

fs/notify/inotify/inotify.h +9 -0

+#ifdef INOTIFY_USE_FHANDLE
+static int inotify_encode_wd_fhandle(struct inotify_inode_mark *mark, struct dentry *dentry)
+{
+	struct file_handle *fhandle = (struct file_handle *)mark->fhandle;
+	int size, ret;
+
+	BUILD_BUG_ON(sizeof(mark->fhandle) <= sizeof(struct file_handle));
+
+	fhandle->handle_bytes = sizeof(mark->fhandle) - sizeof(struct file_handle);

fs/notify/inotify/inotify.h

Quoted 4 lines

fs/notify/inotify/inotify.h +4 -0

+	size = fhandle->handle_bytes >> 2;
+
+	ret = exportfs_encode_fh(dentry, (struct fid *)fhandle->f_handle, &size,  0);
+	if ((ret == 255) || (ret == -ENOSPC))

Sigh. ENOSPC means "your disk is full". If this error ever gets back to userspace, our poor user will go and provision more disks and then wonder why that didn't fix anything.

fs/notify/inotify/inotify.h

Quoted 2 lines

fs/notify/inotify/inotify.h +1 -0

+		return -EOVERFLOW;

That doesn't seem very appropriate either, unsure.

Quoted 10 lines

fs/notify/inotify/inotify.h struct inotify_event_private_data { · line 23

Quoted 7 lines

struct inotify_inode_mark {

fs/notify/inotify/inotify.h +3 -0

 	struct fsnotify_mark fsn_mark;
 	int wd;
+#ifdef INOTIFY_USE_FHANDLE
+	__u8 fhandle[sizeof(struct file_handle) + MAX_HANDLE_SZ];
+#endif
 };

Whoa. This adds 128+8 bytes to the inotify_inode_mark. That's a lot of bloat, and there can be a lot of inotify_inode_mark's in the system?

Yes, that's why it's not turned on by default, only if is c/r turned on. iirc I've been said that usually only about 40 bytes is used (in the tests I met only 8 bytes). Letme re-check all things.

Quoted 7 lines · On Tue, 13 Nov 2012 11:20:57 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 3 lines

Whoa. This adds 128+8 bytes to the inotify_inode_mark. That's a lot of bloat, and there can be a lot of inotify_inode_mark's in the system?

Yes, that's why it's not turned on by default, only if is c/r turned on. iirc I've been said that usually only about 40 bytes is used (in the tests I met only 8 bytes). Letme re-check all things.

The question is, how many `struct inotify_inode_mark's are instantiated system-wide? Could be millions?

Dumb question: do we really need inotify_inode_mark.fhandle at all? What prevents us from assembling this info on demand when ->show_fdinfo() is called?

Quoted 2 lines

The question is, how many `struct inotify_inode_mark's are instantiated system-wide? Could be millions?

Well, hard to tell, to be fair. On my testing machine only apache has been using inotify system as far as I remember, but for sure nothing except memory limit the number of inotify. But I think if one running machine with millions of inotify it's rather powerful machine with enough memory.

Quoted 8 lines · Cyrill Gorcunov

Quoted 2 lines

The question is, how many `struct inotify_inode_mark's are instantiated system-wide? Could be millions?

Well, hard to tell, to be fair. On my testing machine only apache has been using inotify system as far as I remember, but for sure nothing except memory limit the number of inotify. But I think if one running machine with millions of inotify it's rather powerful machine with enough memory.

Seems easy to determine if you boot with slub_nomerge on the command line and then read /sys/kernel/slab/inotify_inode_mark/objects. On my system, that happens to be 210, but I'm sure you could come up with a realistic synthetic workload to make it much higher.

Quoted 2 lines

Which would give about 26K of additional memory if c/r get used here.
Not a big number i guess?

I am pretty sure there are desktop file indexing packages which use inotify or fanotify which will put a mark on every single directory within users home.

You probably need to test this with default installs of popular desktop environments and realistic home directories.

Quoted 8 lines · Tvrtko Ursulin

Quoted 3 lines

Which would give about 26K of additional memory if c/r get used here.

Not a big number i guess?

I am pretty sure there are desktop file indexing packages which use inotify or fanotify which will put a mark on every single directory within users home.

Quoted 2 lines

You probably need to test this with default installs of popular desktop environments and realistic home directories.

I'm about to shrink the handle down to 40/64 bytes as being proposed in one of early review cycles (i'll do that with patch on top), which should minimize the amount of memory needed (look, it's pretty clear that if the system uses millions of inotify watchers each inotify mark will need the fhandle here in c/r sake, i simply see no way at moment how to escape this

completely, but if the c/r is turned off, which is by default, no additional memory needed).

p.s.: could you please don't use the html formatted messages

Quoted 9 lines · On Tuesday 13 November 2012 18:40:36 Cyrill Gorcunov wrote:

Quoted 8 lines · Tvrtko Ursulin

Quoted 3 lines

Which would give about 26K of additional memory if c/r get used here.

Not a big number i guess?

I am pretty sure there are desktop file indexing packages which use inotify or fanotify which will put a mark on every single directory within users home.

Quoted 8 lines

Quoted 2 lines

You probably need to test this with default installs of popular desktop environments and realistic home directories.

I'm about to shrink the handle down to 40/64 bytes as being proposed in one of early review cycles (i'll do that with patch on top), which should minimize the amount of memory needed (look, it's pretty clear that if the system uses millions of inotify watchers each inotify mark will need the fhandle here in c/r sake, i simply see no way at moment how to escape this

Well I spotted uncertainty in this thread about how many of these structures will typically be instantiated at runtime which is what I tried to add to this discussion. I have 60k directories in my home for example...

If you don't want to use information I provided that is your choice. Because you are still probably doubling this structure. Give or take - I haven't actually bothered counting.

Perhaps there could be a different way, where you could use additional space only when it is actually used at runtime. But as I said, I am not following closely.

Quoted 3 lines

completely, but if the c/r is turned off, which is by default, no additional memory needed).

By turned on and off you are not talking about runtime but about kernel compile time, right? Do you envisage distributions turning this on, like they do for most things?

Quoted 3 lines · Tvrtko Ursulin

Perhaps there could be a different way, where you could use additional space only when it is actually used at runtime. But as I said, I am not following closely.

Unfortunatelly at moment I see no way how to make it in on-demand fashion.

Quoted 6 lines · On Tuesday 13 November 2012 19:28:46 Cyrill Gorcunov wrote:

Quoted 3 lines · Tvrtko Ursulin

Perhaps there could be a different way, where you could use additional space only when it is actually used at runtime. But as I said, I am not following closely.

Unfortunatelly at moment I see no way how to make it in on-demand fashion.

You could not use a pointer and then allocate your buffers on the check point operation, freeing on restore?

Quoted 10 lines · Tvrtko Ursulin

Quoted 8 lines · On Tuesday 13 November 2012 19:28:46 Cyrill Gorcunov wrote:

Quoted 4 lines · Tvrtko Ursulin

Perhaps there could be a different way, where you could use additional space only when it is actually used at runtime. But as I said, I am not
following closely.

Unfortunatelly at moment I see no way how to make it in on-demand fashion.

Quoted 2 lines

You could not use a pointer and then allocate your buffers on the check point operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

Quoted 8 lines

Quoted 2 lines

You could not use a pointer and then allocate your buffers on the check point operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right?

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

Quoted 11 lines · Tvrtko Ursulin

Quoted 9 lines

Quoted 3 lines

You could not use a pointer and then allocate your buffers on the check
point operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right?

dentry only needed to encode the file handle.

Quoted 2 lines

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way to restore path from inode solely. or there something i miss?

Quoted 13 lines · On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:

Quoted 11 lines · Tvrtko Ursulin

Quoted 9 lines

Quoted 3 lines

You could not use a pointer and then allocate your buffers on the check
point operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right?

Quoted 7 lines

dentry only needed to encode the file handle.

Quoted 2 lines

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way to restore path from inode solely. or there something i miss?

I don't know, as I said I was not following this at all until now. Just throwing in ideas.

I thought, since inotify does not use the path or dentry outside the system call at all, perhaps you need a different entry point allowing you to restore the watch using the inode or something. Assuming life time of objects and stuff in C&R world would allow you that. Since you don't need the full path, just something 64 bytes long, I assumed that could be the case.

Regards,

Tvrtko

.

Quoted 26 lines · Tvrtko Ursulin

Quoted 14 lines · On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:

Quoted 4 lines · Tvrtko Ursulin

Quoted 4 lines

Quoted 3 lines

You could not use a pointer and then allocate your buffers on the check
point operation, freeing on restore?

Quoted 8 lines

Quoted 6 lines

The problem is not allocating the memory itself but rather the time when
the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right?

Quoted 7 lines

dentry only needed to encode the file handle.

Quoted 2 lines

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way to restore path from inode solely. or there something i miss?

I don't know, as I said I was not following this at all until now. Just throwing in ideas.

Quoted 7 lines

I thought, since inotify does not use the path or dentry outside the system
call at all, perhaps you need a different entry point allowing you to restore the watch using the inode or something. Assuming life time of objects and stuff in C&R world would allow you that. Since you don't need the full path, just something 64 bytes long, I assumed that could be the case.

Well, the kernel already has all the API we need but one -- it shows us _nothing_ about the inode being watched. And we'd appreciate any information about it. Even the ino:dev pair would work. We propose to show the handle because we believe, that such API is better that ino:dev. You can get the handle, call the open_by_handle_at right at once and get much much more information about the inode with any other API (e.g. calling fstat() will give you the ino:dev pair). Having just ino:dev pair at hands is not that flexible.

Quoted 35 lines · On Wednesday 14 November 2012 14:13:41 Pavel Emelyanov wrote:

Quoted 26 lines · Tvrtko Ursulin

Quoted 14 lines · On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:

Quoted 4 lines · Tvrtko Ursulin

Quoted 4 lines

Quoted 3 lines

You could not use a pointer and then allocate your buffers on the check
point operation, freeing on restore?

Quoted 8 lines

Quoted 6 lines

The problem is not allocating the memory itself but rather the time when
the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At any later point we simply have no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right?

Quoted 7 lines

dentry only needed to encode the file handle.

Quoted 2 lines

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way to restore path from inode solely. or there something i miss?

I don't know, as I said I was not following this at all until now. Just throwing in ideas.

Quoted 7 lines

I thought, since inotify does not use the path or dentry outside the system
call at all, perhaps you need a different entry point allowing you to restore the watch using the inode or something. Assuming life time of objects and stuff in C&R world would allow you that. Since you don't need the full path, just something 64 bytes long, I assumed that could be the case.

Quoted 8 lines

Well, the kernel already has all the API we need but one -- it shows us _nothing_ about the inode being watched. And we'd appreciate any information about it. Even the ino:dev pair would work. We propose to show the handle because we believe, that such API is better that ino:dev. You can get the handle, call the open_by_handle_at right at once and get much much more information about the inode with any other API (e.g. calling fstat() will give you the ino:dev pair). Having just ino:dev pair at hands is not that flexible.

How much space does a typical file system need to encode a handle? Am I right that for must it is just a few bytes? (I just glanced at the code so I might be wrong.) In which case, could the handle buffer be allocated dynamically depending on the underlying filesystem? Perhaps adding a facility to query a filesystem about its maximum handle buffer needs? Do you think the saving would justify this extra work?

Regards,

Tvrtko

Quoted 50 lines · Tvrtko Ursulin

Quoted 38 lines · On Wednesday 14 November 2012 14:13:41 Pavel Emelyanov wrote:

Quoted 28 lines · Tvrtko Ursulin

Quoted 16 lines · On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:

Quoted 12 lines · Tvrtko Ursulin

Quoted 11 lines

Quoted 3 lines

You could not use a pointer and then allocate your buffers on the check
point operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when
the information needed (ie the dentry) is available. The only moment when we can use dentry of the target file/directory is at inotify_new_watch, that's why i need to compose fhandle that early. At
any later point we simply have no dentry to use.

Quoted 2 lines

But you do not fundamentally need the dentry to restore a watch, right?

Quoted 7 lines

dentry only needed to encode the file handle.

Quoted 2 lines

Couldn't you restore, creating a new restore path if needed, using the inode which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way to restore path from inode solely. or there something i miss?

I don't know, as I said I was not following this at all until now. Just throwing in ideas.

Quoted 8 lines

I thought, since inotify does not use the path or dentry outside the system
call at all, perhaps you need a different entry point allowing you to restore the watch using the inode or something. Assuming life time of objects and stuff in C&R world would allow you that. Since you don't need
the full path, just something 64 bytes long, I assumed that could be the case.

Quoted 10 lines

Well, the kernel already has all the API we need but one -- it shows us _nothing_ about the inode being watched. And we'd appreciate any information about it. Even the ino:dev pair would work. We propose to show
the handle because we believe, that such API is better that ino:dev. You can get the handle, call the open_by_handle_at right at once and get much much more information about the inode with any other API (e.g. calling fstat() will give you the ino:dev pair). Having just ino:dev pair at hands
is not that flexible.

Quoted 6 lines

How much space does a typical file system need to encode a handle? Am I right that for must it is just a few bytes? (I just glanced at the code so I might be wrong.) In which case, could the handle buffer be allocated dynamically depending on the underlying filesystem? Perhaps adding a facility to query a filesystem about its maximum handle buffer needs? Do you think the saving would justify this extra work?

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite big for inotify extension indeed. The good news is that this amount of bytes seem to be required for the most descriptive fhandle -- with info about parent, etc. We don't need such, we can live with shorter handle, people said that 40 bytes was enough for that.

However, your idea about determining the handle size dynamically seems promising. As far as I can see from the code we can call for encode_fh with size equals zero and filesystem would report back the amount of bytes it requires for a handle.

We can try going this route, what do you think?

Quoted 18 lines

Quoted 6 lines

How much space does a typical file system need to encode a handle? Am I right that for must it is just a few bytes? (I just glanced at the code so I might be wrong.) In which case, could the handle buffer be allocated dynamically depending on the underlying filesystem? Perhaps adding a facility to query a filesystem about its maximum handle buffer needs? Do you think the saving would justify this extra work?

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite big for inotify extension indeed. The good news is that this amount of bytes seem to be required for the most descriptive fhandle -- with info about parent, etc. We don't need such, we can live with shorter handle, people said that 40 bytes was enough for that.

However, your idea about determining the handle size dynamically seems promising. As far as I can see from the code we can call for encode_fh with size equals zero and filesystem would report back the amount of bytes it requires for a handle.

Quoted 1 line

We can try going this route, what do you think?

Sounds much better since that would only add one pointer to the watch structure in the normal case.

Also at checkpoint time it will use only a few bytes (compared to 64) for the encode buffer for most filesystems. This part is probably not that important but still a win.

Regards,

Tvrtko

Quoted 23 lines

Quoted 13 lines

Quoted 6 lines

How much space does a typical file system need to encode a handle? Am I right that for must it is just a few bytes? (I just glanced at the code so I might be wrong.) In which case, could the handle buffer be allocated dynamically depending on the underlying filesystem? Perhaps adding a facility to query a filesystem about its maximum handle buffer needs? Do you think the saving would justify this extra work?

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite big for inotify extension indeed. The good news is that this amount of bytes seem to be required for the most descriptive fhandle -- with info about parent, etc. We don't need such, we can live with shorter handle, people said that 40 bytes was enough for that.

Quoted 6 lines

However, your idea about determining the handle size dynamically seems promising. As far as I can see from the code we can call for encode_fh with size equals zero and filesystem would report back the amount of bytes it requires for a handle.

We can try going this route, what do you think?

Sounds much better since that would only add one pointer to the watch structure in the normal case.

Quoted 3 lines

Also at checkpoint time it will use only a few bytes (compared to 64) for the encode buffer for most filesystems. This part is probably not that important but still a win.

No, the thing is -- we need to know the handle _before_ we start checkpoint. More exactly -- at the time the inotify_add_watch is called. So the memory save would be not that big.

Quoted 26 lines

Quoted 15 lines

Quoted 7 lines

How much space does a typical file system need to encode a handle? Am I right that for must it is just a few bytes? (I just glanced at the code so I might be wrong.) In which case, could the handle buffer be allocated
dynamically depending on the underlying filesystem? Perhaps adding a facility to query a filesystem about its maximum handle buffer needs? Do you think the saving would justify this extra work?

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
big for inotify extension indeed. The good news is that this amount of bytes seem to be required for the most descriptive fhandle -- with info about parent, etc. We don't need such, we can live with shorter handle, people said that 40 bytes was enough for that.

Quoted 7 lines

However, your idea about determining the handle size dynamically seems promising. As far as I can see from the code we can call for encode_fh with
size equals zero and filesystem would report back the amount of bytes it requires for a handle.

We can try going this route, what do you think?

Sounds much better since that would only add one pointer to the watch structure in the normal case.

Quoted 3 lines

Also at checkpoint time it will use only a few bytes (compared to 64) for the encode buffer for most filesystems. This part is probably not that important but still a win.

No, the thing is -- we need to know the handle _before_ we start checkpoint. More exactly -- at the time the inotify_add_watch is called. So the memory save would be not that big.

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite big for inotify extension indeed. The good news is that this amount of bytes seem to be required for the most descriptive fhandle

-- with info about parent, etc. We don't need such, we can live with shorter handle, people said that 40 bytes was enough for that.

However, your idea about determining the handle size dynamically seems promising. As far as I can see from the code we can call for encode_fh with size equals zero and filesystem would report back the amount of bytes it requires for a handle.

We can try going this route, what do you think?

Quoted 2 lines

We can try going this route, what do you think?

I still don't understand why you need a dentry to get the filehandle. The current api may ask for one, but it shouldn't really be necessary (assuming you don't want parent directory information encoded in the filehandle, which I hope you don't).

Quoted 7 lines · J. Bruce Fields

Quoted 2 lines

We can try going this route, what do you think?

I still don't understand why you need a dentry to get the filehandle. The current api may ask for one, but it shouldn't really be necessary (assuming you don't want parent directory information encoded in the filehandle, which I hope you don't).

As far as I know we don't need parent encoded. So Bruce, you think to modify exportfs instead to work with inode directly?

Quoted 11 lines · Cyrill Gorcunov

Quoted 7 lines · J. Bruce Fields

Quoted 2 lines

We can try going this route, what do you think?

I still don't understand why you need a dentry to get the filehandle. The current api may ask for one, but it shouldn't really be necessary (assuming you don't want parent directory information encoded in the filehandle, which I hope you don't).

As far as I know we don't need parent encoded. So Bruce, you think to modify exportfs instead to work with inode directly?

Looks like the filesystem encode_fh method just takes inodes (with the parent inode allowed to be NULL), so all you'd need would be a version of exportfs_encode_fh that took an inode.

(Worst case, if that didn't work, you could fake up a dentry with something like d_obtain_alias, but better not to if it's not necessary.)

Quoted 3 lines

Dumb question: do we really need inotify_inode_mark.fhandle at all? What prevents us from assembling this info on demand when ->show_fdinfo() is called?

exportfs requires the dentry to be passed as an argument while inotify works with inodes instead and at moment of show-fdinfo the target dentry might be already deleted but inode yet present as far as I remember.

Quoted 4 lines · On Tue, 13 Nov 2012 12:00:32 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 3 lines

Dumb question: do we really need inotify_inode_mark.fhandle at all? What prevents us from assembling this info on demand when ->show_fdinfo() is called?

Quoted 3 lines

exportfs requires the dentry to be passed as an argument while inotify works with inodes instead and at moment of show-fdinfo the target dentry might be already deleted but inode yet present as far as I remember.

How can the c/r restore code reestablish the inode data if the dentry isn't there any more?

Quoted 11 lines · Andrew Morton

Quoted 7 lines · On Tue, 13 Nov 2012 12:00:32 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 3 lines

Dumb question: do we really need inotify_inode_mark.fhandle at all? What prevents us from assembling this info on demand when ->show_fdinfo() is called?

exportfs requires the dentry to be passed as an argument while inotify works with inodes instead and at moment of show-fdinfo the target dentry might be already deleted but inode yet present as far as I remember.

Quoted 2 lines

How can the c/r restore code reestablish the inode data if the dentry isn't there any more?

By "deleted" I meant deleted from dcache, thus when we call for open_by_handle_at with fhandle, the kernel reconstruct the path and we simply read the /proc/self/fd/ link, and then pass this path to inotify_add_watch.

.

Quoted 8 lines

Quoted 3 lines

How can the c/r restore code reestablish the inode data if the dentry isn't there any more?

By "deleted" I meant deleted from dcache, thus when we call for open_by_handle_at with fhandle, the kernel reconstruct the path and we simply read the /proc/self/fd/ link, and then pass this path to inotify_add_watch.

No we don't do readlink as the path we'd see would be empty. Instead after we called the open_by_handle_at, we pass the "/proc/self/fd/<fd>" _path_ itself to inotify_add_watch. The path resolution code follows the link properly and adds the target inode into the watch list.

This allow us to print out fsnotify details such as watchee inode, device, mask and optionally a file handle.

For inotify objects if kernel compiled with exportfs support the output will be

 | pos:	0
 | flags:	02000000
 | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 7e9e0000640d1b6d
 | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 11a1000020542153
 | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 49b1060023552153

If kernel compiled without exportfs support, the file handle won't be provided but inode and device only.

 | pos:	0
 | flags:	02000000
 | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0
 | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0
 | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0

For fanotify the output is like

 | pos:	0
 | flags:	02
 | fanotify ino:            68f71 sdev:   800013 mask:        1 ignored_mask: 40000000
 | fanotify mnt_id:       13 mask:        1 ignored_mask: 40000000

To minimize impact on general fsnotify code the new functionality is gathered in fs/notify/fdinfo.c file.

...

fs/notify/fdinfo.h new file +7 -0

@@ -0,0 +1,22 @@
+#ifndef __FSNOTIFY_FDINFO_H__
+#define __FSNOTIFY_FDINFO_H__
+
+#include <linux/errno.h>
+#include <linux/proc_fs.h>
+
+struct seq_file;

fs/notify/fdinfo.h new file +4 -0

+
 
...

@@ -335,6 +336,9 @@ static long inotify_ioctl(struct file *f
 }
 
 static const struct file_operations inotify_fops = {
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= inotify_show_fdinfo,
+#endif
 	.poll		= inotify_poll,
 	.read		= inotify_read,
 	.fasync		= inotify_fasync,

Quoted 9 lines

static const struct file_operations inotify_fops = {

fs/notify/inotify/inotify_user.c +3 -0

+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= inotify_show_fdinfo,
+#endif
 	.poll		= inotify_poll,
 	.read		= inotify_read,
 	.fasync		= inotify_fasync,

fs/notify/inotify/inotify_user.c static long fanotify_ioctl(struct file * · line 436

Quoted 9 lines

fs/notify/inotify/inotify_user.c +3 -0

@@ -427,6 +428,9 @@ static long fanotify_ioctl(struct file *
 }
 
 static const struct file_operations fanotify_fops = {
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= fanotify_show_fdinfo,
+#endif
 	.poll		= fanotify_poll,
 	.read		= fanotify_read,
 	.write		= fanotify_write,

Then remove these ifdefs.

That's if you can be bothered. It's a bit of a party trick which doesn't make things much clearer IMO.