[PATCH 0/4] Series short description

[PATCH 0/4] Series short description

[Alexey Zaytsev]

Hi.

So, it's time for the second version of the the fanotify
range patches.

The patch adds modification ranges to fsnotify events. Fanotify
is made to pass the range to the users.

This is useful for backup programs that work on huge files,
so that only a part of a modified file needs to be scanned
for changes.

changed. This is particularly useful for backup utilities that
work on huge files, so that only a part of the modified file
needs to be scanned for changes.

The series is split into 4 patches. The first one breaks the
ABI (but not API), and should get in before the .37 release,
or not at all. The last patch extends the ABI to handle event-
specific data ('options') in a backwards-compatible way.

You can also get the patchies from

git://git.zaytsev.su/git/linux-2.6.git branch fsnotify

A modified fanotify-example is available from

git://git.zaytsev.su/git/fanotify-example.git branch range

Changes since the first RFC:

1) Reworked the user interface, see the last patch.

2) Handle event merges properly.

3) Pass the range to fsnotify_parent().

3) Various small cleanups and fixes.

---

Alexey Zaytsev (4):
      fanotify: Shrink struct fanotify_event_metadata by 32 bits
      VFS: Tell fsnotify what part of the file might have changed
      fsnotify: Handle the file change ranges
      fanotify: Expose the file changes to the user


 fs/compat.c                        |    2 -
 fs/nfsd/vfs.c                      |    2 -
 fs/notify/fanotify/fanotify.c      |   19 +++++
 fs/notify/fanotify/fanotify_user.c |  132 +++++++++++++++++++++++++++++++-----
 fs/notify/fsnotify.c               |   24 ++++---
 fs/notify/inode_mark.c             |    2 -
 fs/notify/inotify/inotify_user.c   |    2 -
 fs/notify/notification.c           |   18 ++++-
 fs/open.c                          |    4 +
 fs/read_write.c                    |    4 +
 include/linux/fanotify.h           |   88 +++++++++++++++++++++---
 include/linux/fs.h                 |   14 ++++
 include/linux/fsnotify.h           |   68 +++++++++++--------
 include/linux/fsnotify_backend.h   |   31 +++++++-
 14 files changed, 330 insertions(+), 80 deletions(-)

--

[PATCH 1/4] fanotify: Shrink struct fanotify_event_metadata by 32 bits

[Alexey Zaytsev]

And this saves additional 32 bits in the future

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 include/linux/fanotify.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 0f01214..9a7986f 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -86,10 +86,11 @@
 #define FANOTIFY_METADATA_VERSION	2
 
 struct fanotify_event_metadata {
-	__u32 event_len;
-	__u32 vers;
-	__aligned_u64 mask;
+	__u16 event_len;
+	__u8 vers;
+	__u8 reserved;
 	__s32 fd;
+	__aligned_u64 mask;
 	__s32 pid;
 };
 

[PATCH 2/4] VFS: Tell fsnotify what part of the file might have changed

[Alexey Zaytsev]

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/compat.c              |    2 +
 fs/nfsd/vfs.c            |    2 +
 fs/open.c                |    4 +++
 fs/read_write.c          |    4 +--
 include/linux/fs.h       |   14 +++++++++
 include/linux/fsnotify.h |   68 ++++++++++++++++++++++++++++------------------
 6 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index c580c32..66eb689 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1171,7 +1171,7 @@ out:
 		if (type == READ)
 			fsnotify_access(file);
 		else
-			fsnotify_modify(file);
+			fsnotify_modify(file, pos - ret, ret);
 	}
 	return ret;
 }
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 184938f..d781014 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1035,7 +1035,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 		goto out_nfserr;
 	*cnt = host_err;
 	nfsdstats.io_write += host_err;
-	fsnotify_modify(file);
+	fsnotify_modify(file, offset - host_err, host_err);
 
 	/* clear setuid/setgid flag after write */
 	if (inode->i_mode & (S_ISUID | S_ISGID))
diff --git a/fs/open.c b/fs/open.c
index 4197b9e..17b0d79 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -675,6 +675,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	f->f_path.mnt = mnt;
 	f->f_pos = 0;
 	f->f_op = fops_get(inode->i_fop);
+#ifdef CONFIG_FSNOTIFY
+	f->f_whatchanged.start = -1;
+	f->f_whatchanged.end = 0;
+#endif
 	file_sb_list_add(f, inode->i_sb);
 
 	error = security_dentry_open(f, cred);
diff --git a/fs/read_write.c b/fs/read_write.c
index 5d431ba..86c60c2 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -383,7 +383,7 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
 		else
 			ret = do_sync_write(file, buf, count, pos);
 		if (ret > 0) {
-			fsnotify_modify(file);
+			fsnotify_modify(file, (*pos) - ret, ret);
 			add_wchar(current, ret);
 		}
 		inc_syscw(current);
@@ -699,7 +699,7 @@ out:
 		if (type == READ)
 			fsnotify_access(file);
 		else
-			fsnotify_modify(file);
+			fsnotify_modify(file, (*pos) - ret, ret);
 	}
 	return ret;
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index eedc00b..3337975 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -922,6 +922,16 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
 		index <  ra->start + ra->size);
 }
 
+/* 
+ * fsnotify wants to know, what might have changed during the file's lifetime.
+ * We maintain a single range, so it might include non-modified gaps, but this
+ * should work good enough for the majoroty of use cases, and the rest
+ * shold listen to individual 'modify' events. */
+struct fsnotify_range {
+	loff_t start; /* The beginning of the possibly modified area.*/
+	loff_t end; /* The first byte after the area. */
+};
+
 #define FILE_MNT_WRITE_TAKEN	1
 #define FILE_MNT_WRITE_RELEASED	2
 
@@ -965,6 +975,10 @@ struct file {
 #ifdef CONFIG_DEBUG_WRITECOUNT
 	unsigned long f_mnt_write_state;
 #endif
+
+#ifdef CONFIG_FSNOTIFY
+	struct fsnotify_range f_whatchanged;
+#endif
 };
 
 #define get_file(x)	atomic_long_inc(&(x)->f_count)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 5c185fa..49e7788 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -26,12 +26,13 @@ static inline void fsnotify_d_instantiate(struct dentry *dentry,
 }
 
 /* Notify this dentry's parent about a child's events. */
-static inline int fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
+static inline int fsnotify_parent(struct path *path, struct dentry *dentry,
+		__u32 mask, struct fsnotify_range *range)
 {
 	if (!dentry)
 		dentry = path->dentry;
 
-	return __fsnotify_parent(path, dentry, mask);
+	return __fsnotify_parent(path, dentry, mask, range);
 }
 
 /* simple call site for access decisions */
@@ -53,11 +54,12 @@ static inline int fsnotify_perm(struct file *file, int mask)
 	else
 		BUG();
 
-	ret = fsnotify_parent(path, NULL, fsnotify_mask);
+	ret = fsnotify_parent(path, NULL, fsnotify_mask, NULL);
 	if (ret)
 		return ret;
 
-	return fsnotify(inode, fsnotify_mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+	return fsnotify(inode, fsnotify_mask, path, FSNOTIFY_EVENT_PATH,
+			NULL, 0, NULL);
 }
 
 /*
@@ -78,7 +80,7 @@ static inline void fsnotify_d_move(struct dentry *dentry)
  */
 static inline void fsnotify_link_count(struct inode *inode)
 {
-	fsnotify(inode, FS_ATTRIB, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify(inode, FS_ATTRIB, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
 }
 
 /*
@@ -102,14 +104,17 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 		new_dir_mask |= FS_ISDIR;
 	}
 
-	fsnotify(old_dir, old_dir_mask, old_dir, FSNOTIFY_EVENT_INODE, old_name, fs_cookie);
-	fsnotify(new_dir, new_dir_mask, new_dir, FSNOTIFY_EVENT_INODE, new_name, fs_cookie);
+	fsnotify(old_dir, old_dir_mask, old_dir, FSNOTIFY_EVENT_INODE,
+			old_name, fs_cookie, NULL);
+	fsnotify(new_dir, new_dir_mask, new_dir, FSNOTIFY_EVENT_INODE,
+			new_name, fs_cookie, NULL);
 
 	if (target)
 		fsnotify_link_count(target);
 
 	if (source)
-		fsnotify(source, FS_MOVE_SELF, moved->d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+		fsnotify(source, FS_MOVE_SELF, moved->d_inode, FSNOTIFY_EVENT_INODE,
+				NULL, 0, NULL);
 	audit_inode_child(moved, new_dir);
 }
 
@@ -139,7 +144,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
 	if (isdir)
 		mask |= FS_ISDIR;
 
-	fsnotify_parent(NULL, dentry, mask);
+	fsnotify_parent(NULL, dentry, mask, NULL);
 }
 
 /*
@@ -147,7 +152,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
  */
 static inline void fsnotify_inoderemove(struct inode *inode)
 {
-	fsnotify(inode, FS_DELETE_SELF, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify(inode, FS_DELETE_SELF, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
 	__fsnotify_inode_delete(inode);
 }
 
@@ -158,7 +163,8 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
 {
 	audit_inode_child(dentry, inode);
 
-	fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
+	fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE,
+			dentry->d_name.name, 0, NULL);
 }
 
 /*
@@ -171,7 +177,8 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct
 	fsnotify_link_count(inode);
 	audit_inode_child(new_dentry, dir);
 
-	fsnotify(dir, FS_CREATE, inode, FSNOTIFY_EVENT_INODE, new_dentry->d_name.name, 0);
+	fsnotify(dir, FS_CREATE, inode, FSNOTIFY_EVENT_INODE,
+			new_dentry->d_name.name, 0, NULL);
 }
 
 /*
@@ -184,7 +191,8 @@ static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
 
 	audit_inode_child(dentry, inode);
 
-	fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
+	fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE,
+			dentry->d_name.name, 0, NULL);
 }
 
 /*
@@ -200,26 +208,33 @@ static inline void fsnotify_access(struct file *file)
 		mask |= FS_ISDIR;
 
 	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+		fsnotify_parent(path, NULL, mask, NULL);
+		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, NULL);
 	}
 }
 
+
+
 /*
  * fsnotify_modify - file was modified
  */
-static inline void fsnotify_modify(struct file *file)
+static inline void fsnotify_modify(struct file *file, loff_t original, size_t count) 
 {
 	struct path *path = &file->f_path;
 	struct inode *inode = path->dentry->d_inode;
 	__u32 mask = FS_MODIFY;
+	struct fsnotify_range range = {
+		.start = original,
+		.end = original + count,
+	};
 
+	fsnotify_update_range(&file->f_whatchanged, &range);
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
 	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+		fsnotify_parent(path, NULL, mask, &range);
+		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, &range);
 	}
 }
 
@@ -238,8 +253,8 @@ static inline void fsnotify_open(struct file *file)
 	/* FMODE_NONOTIFY must never be set from user */
 	file->f_mode &= ~FMODE_NONOTIFY;
 
-	fsnotify_parent(path, NULL, mask);
-	fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+	fsnotify_parent(path, NULL, mask, NULL);
+	fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, NULL);
 }
 
 /*
@@ -256,8 +271,9 @@ static inline void fsnotify_close(struct file *file)
 		mask |= FS_ISDIR;
 
 	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+		fsnotify_parent(path, NULL, mask, &file->f_whatchanged);
+		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH,
+				NULL, 0, &file->f_whatchanged);
 	}
 }
 
@@ -272,8 +288,8 @@ static inline void fsnotify_xattr(struct dentry *dentry)
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
-	fsnotify_parent(NULL, dentry, mask);
-	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_parent(NULL, dentry, mask, NULL);
+	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
 }
 
 /*
@@ -307,8 +323,8 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
 		if (S_ISDIR(inode->i_mode))
 			mask |= FS_ISDIR;
 
-		fsnotify_parent(NULL, dentry, mask);
-		fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+		fsnotify_parent(NULL, dentry, mask, NULL);
+		fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
 	}
 }
 

[PATCH 3/4] fsnotify: Handle the file change ranges

[Alexey Zaytsev]

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/notify/fsnotify.c             |   24 ++++++++++++++----------
 fs/notify/inode_mark.c           |    2 +-
 fs/notify/inotify/inotify_user.c |    2 +-
 fs/notify/notification.c         |   18 ++++++++++++++++--
 include/linux/fsnotify_backend.h |   31 ++++++++++++++++++++++++++-----
 5 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 20dc218..7cabc1d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -84,7 +84,8 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
 }
 
 /* Notify this dentry's parent about a child's events. */
-int __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
+int __fsnotify_parent(struct path *path, struct dentry *dentry,
+		__u32 mask, struct fsnotify_range *range)
 {
 	struct dentry *parent;
 	struct inode *p_inode;
@@ -108,10 +109,10 @@ int __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
 
 		if (path)
 			ret = fsnotify(p_inode, mask, path, FSNOTIFY_EVENT_PATH,
-				       dentry->d_name.name, 0);
+				       dentry->d_name.name, 0, range);
 		else
 			ret = fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE,
-				       dentry->d_name.name, 0);
+				       dentry->d_name.name, 0, range);
 	}
 
 	dput(parent);
@@ -126,6 +127,7 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 			 __u32 mask, void *data,
 			 int data_is, u32 cookie,
 			 const unsigned char *file_name,
+			 struct fsnotify_range *range,
 			 struct fsnotify_event **event)
 {
 	struct fsnotify_group *group = NULL;
@@ -167,10 +169,11 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 
 	pr_debug("%s: group=%p to_tell=%p mnt=%p mask=%x inode_mark=%p"
 		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
-		 " data=%p data_is=%d cookie=%d event=%p\n",
+		 " data=%p data_is=%d cookie=%d range = {%lld, %lld}, event=%p\n",
 		 __func__, group, to_tell, mnt, mask, inode_mark,
 		 inode_test_mask, vfsmount_mark, vfsmount_test_mask, data,
-		 data_is, cookie, *event);
+		 data_is, cookie, range ? range->start : -1,
+		 range ? range->end : -1, *event);
 
 	if (!inode_test_mask && !vfsmount_test_mask)
 		return 0;
@@ -183,7 +186,7 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 	if (!*event) {
 		*event = fsnotify_create_event(to_tell, mask, data,
 						data_is, file_name,
-						cookie, GFP_KERNEL);
+						cookie, range, GFP_KERNEL);
 		if (!*event)
 			return -ENOMEM;
 	}
@@ -197,7 +200,8 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
  * notification event in whatever means they feel necessary.
  */
 int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
-	     const unsigned char *file_name, u32 cookie)
+	     const unsigned char *file_name, u32 cookie,
+	     struct fsnotify_range *range)
 {
 	struct hlist_node *inode_node = NULL, *vfsmount_node = NULL;
 	struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL;
@@ -256,17 +260,17 @@ int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
 		if (inode_group > vfsmount_group) {
 			/* handle inode */
 			ret = send_to_group(to_tell, NULL, inode_mark, NULL, mask, data,
-					    data_is, cookie, file_name, &event);
+					    data_is, cookie, file_name, range, &event);
 			/* we didn't use the vfsmount_mark */
 			vfsmount_group = NULL;
 		} else if (vfsmount_group > inode_group) {
 			ret = send_to_group(to_tell, mnt, NULL, vfsmount_mark, mask, data,
-					    data_is, cookie, file_name, &event);
+					    data_is, cookie, file_name, range, &event);
 			inode_group = NULL;
 		} else {
 			ret = send_to_group(to_tell, mnt, inode_mark, vfsmount_mark,
 					    mask, data, data_is, cookie, file_name,
-					    &event);
+					    range, &event);
 		}
 
 		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c
index 4c29fcf..cd39df7 100644
--- a/fs/notify/inode_mark.c
+++ b/fs/notify/inode_mark.c
@@ -295,7 +295,7 @@ void fsnotify_unmount_inodes(struct list_head *list)
 			iput(need_iput_tmp);
 
 		/* for each watch, send FS_UNMOUNT and then remove it */
-		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
 
 		fsnotify_inode_delete(inode);
 
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 444c305..a5c2c69 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -524,7 +524,7 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 
 	ignored_event = fsnotify_create_event(NULL, FS_IN_IGNORED, NULL,
 					      FSNOTIFY_EVENT_NONE, NULL, 0,
-					      GFP_NOFS);
+					      NULL, GFP_NOFS);
 	if (!ignored_event)
 		return;
 
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index f39260f..20b86a0 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -395,7 +395,8 @@ struct fsnotify_event *fsnotify_clone_event(struct fsnotify_event *old_event)
  */
 struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask, void *data,
 					     int data_type, const unsigned char *name,
-					     u32 cookie, gfp_t gfp)
+					     u32 cookie, struct fsnotify_range *range,
+					     gfp_t gfp)
 {
 	struct fsnotify_event *event;
 
@@ -422,6 +423,19 @@ struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask,
 	event->to_tell = to_tell;
 	event->data_type = data_type;
 
+	/* The range might be allocated on stack. */
+	if (mask & FS_MODIFY) {
+		event->mod_range = *range;
+	} else {
+		event->mod_range.start = -1;
+	}
+
+	if (mask & FS_CLOSE_WRITE) {
+		event->cw_range = *range;
+	} else {
+		event->cw_range.start = -1;
+	}
+
 	switch (data_type) {
 	case FSNOTIFY_EVENT_PATH: {
 		struct path *path = data;
@@ -453,7 +467,7 @@ __init int fsnotify_notification_init(void)
 	fsnotify_event_holder_cachep = KMEM_CACHE(fsnotify_event_holder, SLAB_PANIC);
 
 	q_overflow_event = fsnotify_create_event(NULL, FS_Q_OVERFLOW, NULL,
-						 FSNOTIFY_EVENT_NONE, NULL, 0,
+						 FSNOTIFY_EVENT_NONE, NULL, 0, NULL,
 						 GFP_KERNEL);
 	if (!q_overflow_event)
 		panic("unable to allocate fsnotify q_overflow_event\n");
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 0a68f92..63237c5 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -240,6 +240,9 @@ struct fsnotify_event {
 	size_t name_len;
 	struct pid *tgid;
 
+	struct fsnotify_range mod_range; /* What has been modified last time */
+	struct fsnotify_range cw_range; /* What has been modified since the file was opened */
+
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 	__u32 response;	/* userspace answer to question */
 #endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
@@ -305,8 +308,10 @@ struct fsnotify_mark {
 
 /* main fsnotify call to send events */
 extern int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
-		    const unsigned char *name, u32 cookie);
-extern int __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask);
+		    const unsigned char *name, u32 cookie,
+		    struct fsnotify_range *range);
+extern int __fsnotify_parent(struct path *path, struct dentry *dentry,
+		__u32 mask, struct fsnotify_range *range);
 extern void __fsnotify_inode_delete(struct inode *inode);
 extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
 extern u32 fsnotify_get_cookie(void);
@@ -420,22 +425,34 @@ extern void fsnotify_unmount_inodes(struct list_head *list);
 extern struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask,
 						    void *data, int data_is,
 						    const unsigned char *name,
-						    u32 cookie, gfp_t gfp);
+						    u32 cookie,
+						    struct fsnotify_range *range,
+						    gfp_t gfp);
 
 /* fanotify likes to change events after they are on lists... */
 extern struct fsnotify_event *fsnotify_clone_event(struct fsnotify_event *old_event);
 extern int fsnotify_replace_event(struct fsnotify_event_holder *old_holder,
 				  struct fsnotify_event *new_event);
 
+static inline void fsnotify_update_range(struct fsnotify_range *new,
+		struct fsnotify_range *old)
+{
+	/* Cast because an empty range starts at -1. */
+	new->start = min((unsigned long long) old->start, (unsigned long long) new->start);
+	new->end = max(old->end, new->end);
+}
+
 #else
 
 static inline int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
-			   const unsigned char *name, u32 cookie)
+			   const unsigned char *name, u32 cookie,
+			   struct fsnotify_range *range)
 {
 	return 0;
 }
 
-static inline int __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
+static inline int __fsnotify_parent(struct path *path, struct dentry *dentry,
+		__u32 mask, struct fsnotify_range *range)
 {
 	return 0;
 }
@@ -460,6 +477,10 @@ static inline u32 fsnotify_get_cookie(void)
 static inline void fsnotify_unmount_inodes(struct list_head *list)
 {}
 
+static inline void fsnotify_update_range(struct fsnotify_range *new,
+		struct fsnotify_range *old)
+{}
+
 #endif	/* CONFIG_FSNOTIFY */
 
 #endif	/* __KERNEL __ */

[PATCH 4/4] fanotify: Expose the file changes to the user

[Alexey Zaytsev]

And revamp the event passing interface in the process.

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/notify/fanotify/fanotify.c      |   19 +++++
 fs/notify/fanotify/fanotify_user.c |  132 +++++++++++++++++++++++++++++++-----
 include/linux/fanotify.h           |   85 ++++++++++++++++++++---
 3 files changed, 206 insertions(+), 30 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index b04f88e..1dced4f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -53,9 +53,15 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list,
 
 	fsnotify_get_event(test_event);
 
-	/* if they are exactly the same we are done */
-	if (test_event->mask == event->mask)
+
+	/*
+	 * Event masks are the same, and the event does not
+	 * carry any optional data
+	 */
+	if (test_event->mask == event->mask &&
+		!(event->mask & (FS_MODIFY | FS_CLOSE_WRITE))) {
 		return test_event;
+	}
 
 	/*
 	 * if the refcnt == 2 this is the only queue
@@ -64,6 +70,13 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list,
 	 */
 	if (atomic_read(&test_event->refcnt) == 2) {
 		test_event->mask |= event->mask;
+		/*
+		 * Update the event's ranges, works even if
+		 * the event does not carry any ranges, so don't
+		 * bother checking.
+		 */
+		fsnotify_update_range(&test_event->mod_range, &event->mod_range);
+		fsnotify_update_range(&test_event->cw_range, &event->cw_range);
 		return test_event;
 	}
 
@@ -78,6 +91,8 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list,
 
 	/* build new event and replace it on the list */
 	new_event->mask = (test_event->mask | event->mask);
+	fsnotify_update_range(&new_event->mod_range, &event->mod_range);
+	fsnotify_update_range(&new_event->cw_range, &event->cw_range);
 	fsnotify_replace_event(test_holder, new_event);
 
 	/* we hold a reference on new_event from clone_event */
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 0632248..5774552 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -31,6 +31,57 @@ struct fanotify_response_event {
 	struct fsnotify_event *event;
 };
 
+#ifdef DEBUG
+static inline void dump_event(const char *str, void *buf)
+{
+	struct fanotify_event_metadata *meta = buf;
+	struct fanotify_opt_hdr *tmp;
+
+	pr_debug("%s: event_len=%d vers=%d mask=0x%lld fd=%d pid=%d\n", str,
+		meta->event_len, meta->vers, meta->mask, meta->fd, meta->pid);
+
+	FAN_FOR_EACH_OPTION(meta, tmp) {
+		/* Hope gcc does a good job here. */
+		struct fan_modify_option *m_opt = FAN_OPT_PAYLOAD(tmp);
+		struct fan_close_write_option *cw_opt = FAN_OPT_PAYLOAD(tmp);
+		switch(tmp->type) {
+
+		case FAN_OPT_MODIFY:
+			pr_debug("\tOption: type = %d, len = %d, range {%lld, %lld}\n",
+					tmp->type, tmp->len, m_opt->start, m_opt->end);
+			break;
+
+		case FAN_OPT_CLOSE_WRITE:
+			pr_debug("\tOption: type = %d, len = %d, range {%lld, %lld}\n",
+					tmp->type, tmp->len, cw_opt->start, cw_opt->end);
+			break;
+
+		default:
+			/* Don't forget to add new options here. */
+			WARN_ON_ONCE(1);
+		}
+	}
+}
+#else
+static inline void dump_event(const char *str, void *buf)
+{
+}
+#endif
+
+static inline int fanotify_event_len(struct fsnotify_event *event)
+{
+	int len = sizeof(struct fanotify_event_metadata);
+	u32 mask = event->mask & FAN_ALL_OUTGOING_EVENTS;
+
+	if (mask & FAN_MODIFY)
+		len += (sizeof(struct fanotify_opt_hdr)
+				+ sizeof(struct fan_modify_option));
+	if (mask & FAN_CLOSE_WRITE)
+		len += (sizeof(struct fanotify_opt_hdr)
+				+ sizeof(struct fan_close_write_option));
+	return len;
+}
+
 /*
  * Get an fsnotify notification event if one exists and is small
  * enough to fit in "count". Return an error pointer if the count
@@ -41,6 +92,7 @@ struct fanotify_response_event {
 static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
 {
+	struct fsnotify_event *event;
 	BUG_ON(!mutex_is_locked(&group->notification_mutex));
 
 	pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
@@ -48,7 +100,8 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 	if (fsnotify_notify_queue_is_empty(group))
 		return NULL;
 
-	if (FAN_EVENT_METADATA_LEN > count)
+	event = fsnotify_peek_notify_event(group);
+	if (fanotify_event_len(event) > count)
 		return ERR_PTR(-EINVAL);
 
 	/* held the notification_mutex the whole time, so this is the
@@ -106,20 +159,58 @@ static int create_fd(struct fsnotify_group *group, struct fsnotify_event *event)
 	return client_fd;
 }
 
-static ssize_t fill_event_metadata(struct fsnotify_group *group,
-				   struct fanotify_event_metadata *metadata,
+static int fill_event_metadata(struct fsnotify_group *group,
+				   char *event_buffer, int len,
 				   struct fsnotify_event *event)
 {
-	pr_debug("%s: group=%p metadata=%p event=%p\n", __func__,
-		 group, metadata, event);
+	struct fanotify_event_metadata *meta =
+		(struct fanotify_event_metadata *) event_buffer;
+
+	struct fanotify_opt_hdr *opt_hdr = (struct fanotify_opt_hdr *)(meta + 1);
+	int meta_len = sizeof(struct fanotify_event_metadata);
+
+	pr_debug("%s: group=%p meta=%p event=%p\n", __func__,
+		 group, meta, event);
+
+	if (event->mask & FS_MODIFY) {
+		struct fan_modify_option *opt =
+			(struct fan_modify_option *) (opt_hdr + 1);
+		int opt_len = sizeof(struct fanotify_opt_hdr) +
+			sizeof(struct fan_modify_option);
 
-	metadata->event_len = FAN_EVENT_METADATA_LEN;
-	metadata->vers = FANOTIFY_METADATA_VERSION;
-	metadata->mask = event->mask & FAN_ALL_OUTGOING_EVENTS;
-	metadata->pid = pid_vnr(event->tgid);
-	metadata->fd = create_fd(group, event);
+		meta_len += opt_len;
+		opt_hdr->type = FAN_OPT_MODIFY;
+		opt_hdr->len = opt_len;
+		opt->start = event->mod_range.start;
+		opt->end = event->mod_range.end;
 
-	return metadata->fd;
+		opt_hdr = (struct fanotify_opt_hdr *) (((char *) opt_hdr) + opt_len);
+	}
+	if (event->mask & FS_CLOSE_WRITE) {
+		struct fan_close_write_option *opt =
+			(struct fan_close_write_option *) (opt_hdr + 1);
+		int opt_len = sizeof(struct fanotify_opt_hdr) +
+			sizeof(struct fan_close_write_option);
+
+		meta_len += opt_len;
+		opt_hdr->type = FAN_OPT_CLOSE_WRITE;
+		opt_hdr->len = opt_len;
+		opt->start = event->cw_range.start;
+		opt->end = event->cw_range.end;
+
+		opt_hdr = (struct fanotify_opt_hdr *) (((char *) opt_hdr) + opt_len);
+	}
+
+	WARN_ON_ONCE(len != meta_len);
+
+	meta->event_len = meta_len;
+	meta->vers = FANOTIFY_METADATA_VERSION;
+	meta->options_offset = sizeof(struct fanotify_event_metadata);
+	meta->mask = event->mask & FAN_ALL_OUTGOING_EVENTS;
+	meta->pid = pid_vnr(event->tgid);
+	meta->fd = create_fd(group, event);
+
+	return meta->fd;
 }
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
@@ -248,16 +339,17 @@ static void remove_access_response(struct fsnotify_group *group,
 }
 #endif
 
-static ssize_t copy_event_to_user(struct fsnotify_group *group,
+static int copy_event_to_user(struct fsnotify_group *group,
 				  struct fsnotify_event *event,
 				  char __user *buf)
 {
-	struct fanotify_event_metadata fanotify_event_metadata;
+	int len = fanotify_event_len(event);
+	char fanotify_event_buf[len];
 	int fd, ret;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	fd = fill_event_metadata(group, &fanotify_event_metadata, event);
+	fd = fill_event_metadata(group, fanotify_event_buf, len, event);
 	if (fd < 0)
 		return fd;
 
@@ -266,10 +358,12 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		goto out_close_fd;
 
 	ret = -EFAULT;
-	if (copy_to_user(buf, &fanotify_event_metadata, FAN_EVENT_METADATA_LEN))
+	if (copy_to_user(buf, fanotify_event_buf, len))
 		goto out_kill_access_response;
 
-	return FAN_EVENT_METADATA_LEN;
+	dump_event(__func__, fanotify_event_buf);
+
+	return len;
 
 out_kill_access_response:
 	remove_access_response(group, event, fd);
@@ -418,8 +512,10 @@ static long fanotify_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	switch (cmd) {
 	case FIONREAD:
 		mutex_lock(&group->notification_mutex);
-		list_for_each_entry(holder, &group->notification_list, event_list)
-			send_len += FAN_EVENT_METADATA_LEN;
+		list_for_each_entry(holder, &group->notification_list, event_list) {
+			struct fsnotify_event *event = holder->event;
+			send_len += fanotify_event_len(event);
+		}
 		mutex_unlock(&group->notification_mutex);
 		ret = put_user(send_len, (int __user *) p);
 		break;
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 9a7986f..87dfe36 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -83,17 +83,50 @@
 				 FAN_ALL_PERM_EVENTS |\
 				 FAN_Q_OVERFLOW)
 
-#define FANOTIFY_METADATA_VERSION	2
+/* Option types, mirror the event mask, but we can pack them into u8.  */
+enum fanotify_opt_type {
+	FAN_OPT_NONE = 0,
+	FAN_OPT_ACCESS,
+	FAN_OPT_MODIFY,
+	FAN_OPT_CLOSE_WRITE,
+	FAN_OPT_CLOSE_NOWRITE,
+	FAN_OPT_OPEN,
+	FAN_OPT_Q_OVERFLOW,
+	FAN_OPT_OPEN_PERM,
+	FAN_OPT_ACCESS_PERM,
+};
+
+struct fanotify_opt_hdr {
+	__u8 type;
+	__u8 reserved;
+	__u16 len;
+	/* Payload goes here. */
+};
+
+#define FANOTIFY_METADATA_VERSION	3
 
 struct fanotify_event_metadata {
-	__u16 event_len;
+	__u16 event_len; /* Including the options */
 	__u8 vers;
-	__u8 reserved;
+	__u8 options_offset; /* Aka header length */
 	__s32 fd;
 	__aligned_u64 mask;
 	__s32 pid;
+	/* Options go here. */
 };
 
+struct fan_modify_option {
+	__aligned_u64 start; /* Start of the modification. */
+	__aligned_u64 end; /* The position right after the modification. */
+};
+
+struct fan_close_write_option {
+	__aligned_u64 start; /* Start of the modifications. */
+	__aligned_u64 end; /* The position right after the modifications. */
+};
+
+/* Update fanotify_event_len() when you add more options. */
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;
@@ -103,15 +136,47 @@ struct fanotify_response {
 #define FAN_ALLOW	0x01
 #define FAN_DENY	0x02
 
-/* Helper functions to deal with fanotify_event_metadata buffers */
-#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))
-
 #define FAN_EVENT_NEXT(meta, len) ((len) -= (meta)->event_len, \
 				   (struct fanotify_event_metadata*)(((char *)(meta)) + \
 				   (meta)->event_len))
 
-#define FAN_EVENT_OK(meta, len)	((long)(len) >= (long)FAN_EVENT_METADATA_LEN && \
-				(long)(meta)->event_len >= (long)FAN_EVENT_METADATA_LEN && \
-				(long)(meta)->event_len <= (long)(len))
-
+static inline int FAN_EVENT_OK(struct fanotify_event_metadata *meta, size_t len)
+{
+	return (len >= sizeof(struct fanotify_event_metadata) &&
+		meta->event_len >= sizeof(struct fanotify_event_metadata) &&
+		meta->event_len <= len);
+}
+
+#define FAN_FOR_EACH_EVENT(meta, buf, len) \
+	for (meta = (struct fanotify_event_metadata *) buf; \
+			FAN_EVENT_OK(meta, len); \
+			meta = FAN_EVENT_NEXT(meta, len))
+
+
+static inline struct fanotify_opt_hdr *FAN_OPTION_FIRST(
+		struct fanotify_event_metadata *meta)
+{
+	return (struct fanotify_opt_hdr *) (((char *) meta) + meta->options_offset);
+}
+static inline struct fanotify_opt_hdr *FAN_OPTION_NEXT(
+		struct fanotify_opt_hdr *opt)
+{
+	return (struct fanotify_opt_hdr *) (((char *) opt) + opt->len);
+}
+
+static inline int FAN_OPTION_OK(struct fanotify_event_metadata *meta,
+		struct fanotify_opt_hdr *opt)
+{
+	return ((char *) opt) < (((char *)meta) + meta->event_len);
+}
+
+#define FAN_FOR_EACH_OPTION(meta, tmp) \
+	for (tmp = FAN_OPTION_FIRST(meta); \
+		FAN_OPTION_OK(meta, tmp); \
+		tmp = FAN_OPTION_NEXT(tmp))
+
+static inline void *FAN_OPT_PAYLOAD(struct fanotify_opt_hdr *opt)
+{
+	return (opt + 1);
+}
 #endif /* _LINUX_FANOTIFY_H */

Re: [PATCH 1/4] fanotify: Shrink struct fanotify_event_metadata by 32 bits

[Alexey Zaytsev]

Hi, Eric. Sorry for the noise, but could you please comment at this
one? Because if it goes, it needs to go soon, before the .37 release,
or we won't be able change the ABI.

The other patches in the series are clearly not .37 material, so no
problem if you can't look at them right now.

On Mon, Nov 22, 2010 at 03:33, Alexey Zaytsev <alexey.zaytsev@gmail.com> wrote:
> And this saves additional 32 bits in the future
>
> Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> ---
>  include/linux/fanotify.h |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 0f01214..9a7986f 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -86,10 +86,11 @@
>  #define FANOTIFY_METADATA_VERSION      2
>
>  struct fanotify_event_metadata {
> -       __u32 event_len;
> -       __u32 vers;
> -       __aligned_u64 mask;
> +       __u16 event_len;
> +       __u8 vers;
> +       __u8 reserved;
>        __s32 fd;
> +       __aligned_u64 mask;
>        __s32 pid;
>  };
>
>
>

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Tvrtko Ursulin]

On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
> +struct fanotify_opt_hdr {
> +       __u8 type;
> +       __u8 reserved;
> +       __u16 len;
> +       /* Payload goes here. */
> +};
> +
> +#define FANOTIFY_METADATA_VERSION      3
>
>  struct fanotify_event_metadata {
> -       __u16 event_len;
> +       __u16 event_len; /* Including the options */
>         __u8 vers;
> -       __u8 reserved;
> +       __u8 options_offset; /* Aka header length */
>         __s32 fd;
>         __aligned_u64 mask;
>         __s32 pid;
> +       /* Options go here. */
>  };

I am not 100% comfortable with having 16 bits length fields because I am just
not sure there is a measurable performance difference versus just going with
32 bits.

Also, options_offset is, if I understood it correctly, basically the lenght of
fanotify_event_metadata. As such, isn't that field redundant since the lenght
is implied from the protocol version?

For which it follows that maybe no changes to fanotify_event_metadata are
needed to grow the protocol in the future. We have the version and total
lenght. All that is required is that we never change width and position of
version and lenght field and we should be fine. Later we can define option
structures each of which will "point" to the next and thats it, no?

Tvrtko

Sophos Limited, The Pentagon, Abingdon Science Park, Abingdon, OX14 3YP, United Kingdom.
Company Reg No 2096520. VAT Reg No GB 991 2418 08.

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Alexey Zaytsev]

On Fri, Nov 26, 2010 at 13:11, Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
>> +struct fanotify_opt_hdr {
>> +       __u8 type;
>> +       __u8 reserved;
>> +       __u16 len;
>> +       /* Payload goes here. */
>> +};
>> +
>> +#define FANOTIFY_METADATA_VERSION      3
>>
>>  struct fanotify_event_metadata {
>> -       __u16 event_len;
>> +       __u16 event_len; /* Including the options */
>>         __u8 vers;
>> -       __u8 reserved;
>> +       __u8 options_offset; /* Aka header length */
>>         __s32 fd;
>>         __aligned_u64 mask;
>>         __s32 pid;
>> +       /* Options go here. */
>>  };
>
> I am not 100% comfortable with having 16 bits length fields because I am just
> not sure there is a measurable performance difference versus just going with
> 32 bits.

I'm not concerned so much with the performance, as with the storage.
If we are generating events for every access on a mount point, some
consumers might build a considerable backlog over a period of high
activity. Would be good if we could cut the event size by 1/3 for
free. And I don't see an event growing 64k even with the options. Do
you?


>
> Also, options_offset is, if I understood it correctly, basically the lenght of
> fanotify_event_metadata. As such, isn't that field redundant since the lenght
> is implied from the protocol version?

There are two problems there.

1) You lose backwards-compatibility. It's still an ABI breakage, even
if you tell the users about it.

2) You can't build a program to account for different fanotify versions:
        if (vers >= N) { use the cool stuff } else if {vers >= N-1}  {
still good }

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Tvrtko Ursulin]

On Friday 26 Nov 2010 11:21:18 Alexey Zaytsev wrote:
> On Fri, Nov 26, 2010 at 13:11, Tvrtko Ursulin <tvrtko.ursulin@sophos.com>
wrote:
> > On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
> >> +struct fanotify_opt_hdr {
> >> +       __u8 type;
> >> +       __u8 reserved;
> >> +       __u16 len;
> >> +       /* Payload goes here. */
> >> +};
> >> +
> >> +#define FANOTIFY_METADATA_VERSION      3
> >>
> >>  struct fanotify_event_metadata {
> >> -       __u16 event_len;
> >> +       __u16 event_len; /* Including the options */
> >>         __u8 vers;
> >> -       __u8 reserved;
> >> +       __u8 options_offset; /* Aka header length */
> >>         __s32 fd;
> >>         __aligned_u64 mask;
> >>         __s32 pid;
> >> +       /* Options go here. */
> >>  };
> >
> > I am not 100% comfortable with having 16 bits length fields because I am
> > just not sure there is a measurable performance difference versus just
> > going with 32 bits.
>
> I'm not concerned so much with the performance, as with the storage.
> If we are generating events for every access on a mount point, some
> consumers might build a considerable backlog over a period of high
> activity. Would be good if we could cut the event size by 1/3 for
> free. And I don't see an event growing 64k even with the options. Do
> you?

I don't but maybe it is just lack of imagination.

My bias is that I am mostly thinking about synchronous events where large
backlog is not a realistic scenario. How realistic you think is this with
async events?

> > Also, options_offset is, if I understood it correctly, basically the
> > lenght of fanotify_event_metadata. As such, isn't that field redundant
> > since the lenght is implied from the protocol version?
>
> There are two problems there.
>
> 1) You lose backwards-compatibility. It's still an ABI breakage, even
> if you tell the users about it.

Assuming 2.6.37 release as starting point for ABI considerations?

> 2) You can't build a program to account for different fanotify versions:
>         if (vers >= N) { use the cool stuff } else if {vers >= N-1}  {
> still good }

I don't get why not, but maybe I am just slow today. There will always be 1:1
mapping from version to your options_offset field, no? How does then removing
options_offset change anything?

Tvrtko

Sophos Limited, The Pentagon, Abingdon Science Park, Abingdon, OX14 3YP, United Kingdom.
Company Reg No 2096520. VAT Reg No GB 991 2418 08.

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Alexey Zaytsev]

On Fri, Nov 26, 2010 at 14:41, Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> On Friday 26 Nov 2010 11:21:18 Alexey Zaytsev wrote:
>> On Fri, Nov 26, 2010 at 13:11, Tvrtko Ursulin <tvrtko.ursulin@sophos.com>
> wrote:
>> > On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
>> >> +struct fanotify_opt_hdr {
>> >> +       __u8 type;
>> >> +       __u8 reserved;
>> >> +       __u16 len;
>> >> +       /* Payload goes here. */
>> >> +};
>> >> +
>> >> +#define FANOTIFY_METADATA_VERSION      3
>> >>
>> >>  struct fanotify_event_metadata {
>> >> -       __u16 event_len;
>> >> +       __u16 event_len; /* Including the options */
>> >>         __u8 vers;
>> >> -       __u8 reserved;
>> >> +       __u8 options_offset; /* Aka header length */
>> >>         __s32 fd;
>> >>         __aligned_u64 mask;
>> >>         __s32 pid;
>> >> +       /* Options go here. */
>> >>  };
>> >
>> > I am not 100% comfortable with having 16 bits length fields because I am
>> > just not sure there is a measurable performance difference versus just
>> > going with 32 bits.
>>
>> I'm not concerned so much with the performance, as with the storage.
>> If we are generating events for every access on a mount point, some
>> consumers might build a considerable backlog over a period of high
>> activity. Would be good if we could cut the event size by 1/3 for
>> free. And I don't see an event growing 64k even with the options. Do
>> you?
>
> I don't but maybe it is just lack of imagination.
>
> My bias is that I am mostly thinking about synchronous events where large
> backlog is not a realistic scenario. How realistic you think is this with
> async events?
>

Ok, you are probably right, we can't build a huge backlog, because
each event carries an open fd with it. So, given the unnoticeable
performance gain, it might be worth the leave the possibility to pass
events greater than 64k (with options) in the future, no matter how
unlikely this looks now. We still could save 32 bit by packing the
vers and the offset together. The common part of the events
(options_offset) should not grow over 256 byte, or it would start to
affect the performance for common events, and I hope that 256 versions
would also be enough, so we could even reserve 16 bit for further
extensions.

Eric?

>> > Also, options_offset is, if I understood it correctly, basically the
>> > lenght of fanotify_event_metadata. As such, isn't that field redundant
>> > since the lenght is implied from the protocol version?
>>
>> There are two problems there.
>>
>> 1) You lose backwards-compatibility. It's still an ABI breakage, even
>> if you tell the users about it.
>
> Assuming 2.6.37 release as starting point for ABI considerations?

Yep, that's why I'm asking to include the first patch into .37.

>
>> 2) You can't build a program to account for different fanotify versions:
>>         if (vers >= N) { use the cool stuff } else if {vers >= N-1}  {
>> still good }
>
> I don't get why not, but maybe I am just slow today. There will always be 1:1
> mapping from version to your options_offset field, no? How does then removing
> options_offset change anything?
>

You need to get the old version of the ABI definition somewhere. And
if you need to cast the events to different types, and use different
versions of FAN_OPTION* depending on the event version, the program
also becomes a bit complicated.

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Eric Paris]

On Fri, 2010-11-26 at 10:11 +0000, Tvrtko Ursulin wrote:
> On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:

> >  struct fanotify_event_metadata {
> > -       __u16 event_len;
> > +       __u16 event_len; /* Including the options */
> >         __u8 vers;
> > -       __u8 reserved;
> > +       __u8 options_offset; /* Aka header length */
> >         __s32 fd;
> >         __aligned_u64 mask;
> >         __s32 pid;
> > +       /* Options go here. */
> >  };

> Also, options_offset is, if I understood it correctly, basically the lenght of
> fanotify_event_metadata. As such, isn't that field redundant since the lenght
> is implied from the protocol version?

Ok, the way I envision the interface is the fanotify_event_metadata is
going to be information that is common for all event types.  If a
particular event type (in the case being brought forth MODIFY and
CLOSE_WRITE) needs more information it will use the optional headers.
I'd still like to be able to add generic info, ex: the uid of the
requesting process, to the generic event metadata.  We might also
someday want to use the optional headers for things like the inotify
move cookie.  Why waste space in every event when only the move type
events cared about this cookie?

This necessitates options_offset.  Lets say I compile my userspace app
as version 3.  I know that fanotify_event_metadata is 14 bytes long in
version 3.  Thus I start looking at byte 15 for the first optional
header.  But the kernel is version 4 and fanotify_event_metadata is
actually 18 bytes long.  Whoops, userspace should be looking at byte 19
for the header but accidentally looked at byte 15.  options_offset will
allow a version 3 userspace to run on a version 4 kernel while skipping
bytes 15-18 which it doesn't understand.

At the end of all of this discussion what did people finally agree on?
Something like this?

struct fanotify_opt_hdr {
        __u16 type;
        __u16 len;
        /* Payload goes here. */
};

struct fanotify_event_metadata {
        __u32 event_len; /* Including the options */
        __u16 vers;
        __u16 options_offset; /* Aka header length */
        __s32 fd;
        __aligned_u64 mask;
        __s32 pid;
        /* Options go here. */
};

We could be ABI compatible if we let vers as a __u32 and added
options_offset at the end of the structure...   Do we have strong
feelings on either side about maintaining compatibility or about
structure lengths?

-Eric

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Alexey Zaytsev]

On Mon, Nov 29, 2010 at 19:14, Eric Paris <eparis@redhat.com> wrote:
> On Fri, 2010-11-26 at 10:11 +0000, Tvrtko Ursulin wrote:
>> On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
>
>> >  struct fanotify_event_metadata {
>> > -       __u16 event_len;
>> > +       __u16 event_len; /* Including the options */
>> >         __u8 vers;
>> > -       __u8 reserved;
>> > +       __u8 options_offset; /* Aka header length */
>> >         __s32 fd;
>> >         __aligned_u64 mask;
>> >         __s32 pid;
>> > +       /* Options go here. */
>> >  };
>
>> Also, options_offset is, if I understood it correctly, basically the lenght of
>> fanotify_event_metadata. As such, isn't that field redundant since the lenght
>> is implied from the protocol version?
>
> Ok, the way I envision the interface is the fanotify_event_metadata is
> going to be information that is common for all event types.  If a
> particular event type (in the case being brought forth MODIFY and
> CLOSE_WRITE) needs more information it will use the optional headers.
> I'd still like to be able to add generic info, ex: the uid of the
> requesting process, to the generic event metadata.  We might also
> someday want to use the optional headers for things like the inotify
> move cookie.  Why waste space in every event when only the move type
> events cared about this cookie?
>
> This necessitates options_offset.  Lets say I compile my userspace app
> as version 3.  I know that fanotify_event_metadata is 14 bytes long in
> version 3.  Thus I start looking at byte 15 for the first optional
> header.  But the kernel is version 4 and fanotify_event_metadata is
> actually 18 bytes long.  Whoops, userspace should be looking at byte 19
> for the header but accidentally looked at byte 15.  options_offset will
> allow a version 3 userspace to run on a version 4 kernel while skipping
> bytes 15-18 which it doesn't understand.
>
> At the end of all of this discussion what did people finally agree on?
> Something like this?
>
> struct fanotify_opt_hdr {
>        __u16 type;
>        __u16 len;
>        /* Payload goes here. */
> };
>
> struct fanotify_event_metadata {
>        __u32 event_len; /* Including the options */
>        __u16 vers;
>        __u16 options_offset; /* Aka header length */
>        __s32 fd;
>        __aligned_u64 mask;
>        __s32 pid;
>        /* Options go here. */
> };
>

Yep, except mask should go after options_offset to avoid the gap, and
let's make both vers and options_offset u8, just in case we need the
other 2 bytes in the future:

struct fanotify_event_metadata {
       __u32 event_len; /* Including the options */
       __u8 vers;
       __u8 options_offset; /* Aka header length */
       __u16 reserved;
       __aligned_u64 mask;
       __s32 fd;
       __s32 pid;
       /* Options go here. */
};

> We could be ABI compatible if we let vers as a __u32 and added
> options_offset at the end of the structure...   Do we have strong
> feelings on either side about maintaining compatibility or about3
> structure lengths?

Actually, with event_len being __u32, and mask moved back, we are
already abi-compatible. The users would see the wrong version, but
it's always > 2, so everyone happy. Together with the hypothetical
huge options, this fully justifies spending 2 bytes on event_len to
me.

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Tvrtko Ursulin]

On Monday 29 Nov 2010 16:14:54 Eric Paris wrote:
> On Fri, 2010-11-26 at 10:11 +0000, Tvrtko Ursulin wrote:
> > On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:
> > >  struct fanotify_event_metadata {
> > >
> > > -       __u16 event_len;
> > > +       __u16 event_len; /* Including the options */
> > >
> > >         __u8 vers;
> > >
> > > -       __u8 reserved;
> > > +       __u8 options_offset; /* Aka header length */
> > >
> > >         __s32 fd;
> > >         __aligned_u64 mask;
> > >         __s32 pid;
> > >
> > > +       /* Options go here. */
> > >
> > >  };
> >
> > Also, options_offset is, if I understood it correctly, basically the
> > lenght of fanotify_event_metadata. As such, isn't that field redundant
> > since the lenght is implied from the protocol version?
>
> Ok, the way I envision the interface is the fanotify_event_metadata is
> going to be information that is common for all event types.  If a
> particular event type (in the case being brought forth MODIFY and
> CLOSE_WRITE) needs more information it will use the optional headers.
> I'd still like to be able to add generic info, ex: the uid of the
> requesting process, to the generic event metadata.  We might also
> someday want to use the optional headers for things like the inotify
> move cookie.  Why waste space in every event when only the move type
> events cared about this cookie?
>
> This necessitates options_offset.  Lets say I compile my userspace app
> as version 3.  I know that fanotify_event_metadata is 14 bytes long in
> version 3.  Thus I start looking at byte 15 for the first optional
> header.  But the kernel is version 4 and fanotify_event_metadata is
> actually 18 bytes long.  Whoops, userspace should be looking at byte 19
> for the header but accidentally looked at byte 15.  options_offset will
> allow a version 3 userspace to run on a version 4 kernel while skipping
> bytes 15-18 which it doesn't understand.

I see now what you want to do here. I assumed metadata is fixed for ever and
all expansions will come via options. That is why I said offset field is
redundant. If you want to leave option for changing metadata open then yes, I
agree it is needed.

> At the end of all of this discussion what did people finally agree on?
> Something like this?
>
> struct fanotify_opt_hdr {
>         __u16 type;
>         __u16 len;
>         /* Payload goes here. */
> };
>
> struct fanotify_event_metadata {
>         __u32 event_len; /* Including the options */
>         __u16 vers;
>         __u16 options_offset; /* Aka header length */
>         __s32 fd;
>         __aligned_u64 mask;
>         __s32 pid;
>         /* Options go here. */
> };
>
> We could be ABI compatible if we let vers as a __u32 and added
> options_offset at the end of the structure...   Do we have strong
> feelings on either side about maintaining compatibility or about
> structure lengths?

No strong feelings here on either. I do not really think maintaining
compatibility with an unreleased protocol is worth it. And I would not like to
limit any field to 8-bits if possible.

Tvrtko

Sophos Limited, The Pentagon, Abingdon Science Park, Abingdon, OX14 3YP, United Kingdom.
Company Reg No 2096520. VAT Reg No GB 991 2418 08.

Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Eric Paris]

On Mon, 2010-11-29 at 19:51 +0300, Alexey Zaytsev wrote:

[rewriting history!]
> struct fanotify_event_metadata {
>        __u32 event_len; /* Including the options */
>        __u8 vers;
>        __u8 options_offset; /* Aka header length */
>        __u16 reserved;
>        __aligned_u64 mask;
>        __s32 fd;
>        __s32 pid;
>        /* Options go here. */
> };

> and let's make both vers and options_offset u8, just in case we need the
> other 2 bytes in the future:

So the last discussion is around vers and options_offset.

Alexy:				Tvrtko:
__u8 vers;			__u16 vers;
__u8 options_offset;		__u16 options_offset;
__u16 unused;

The only type of long option that first comes to mind is a filename.   A
filename could easily blow out the __u8 options_offset.  I probably
shouldn't put that in the event_metadata since it wouldn't be fixed
length and it wouldn't allow fixed offset extention of the
event_metadata, so maybe I shouldn't worry about it.  I'm trying to
think of reasons why __u8 isn't adequate other than my usual "just make
it bigger than we ever need"

You'll notice I'm using __u64 for the mask, even though we don't come
close to filling up an __u32 at this point.

Even though I can't think of a likely reason __u8 is bad I think I like
the 'Tvrtko' option better.  I think we should do a compromise:

Eric:
__u8 vers;
__u8 unused;
__u16 options_offset;

If we ever overload vers we can expand into another field.  Would could
change unused into vers2 and define the version as vers + vers2;  vers2
could even exist somewhere else in the metadata.  That can be done once
we get to version 254 while maintaining backwards compat beyond 255.  If
we ever overflow options_offset we are screwed since old userspace
wouldn't know how to handle things.

So, if you want to send me a patch that implements the above (along with
the obvious version bump to 3, I'll queue it up for this merge window
even though we have an ABI compatible solution.

-Eric