Re: [PATCH] [RFC] fsnotify: Tell the user what part of the file mighthave changed

From: Alexey Zaytsev
Date: Mon Nov 15 2010 - 17:59:38 EST

Next message: James Morris: "Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n"
Previous message: James Morris: "Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n"
In reply to: Eric Paris: "Re: [PATCH] [RFC] fsnotify: Tell the user what part of the filemight have changed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Nov 16, 2010 at 01:34, Eric Paris <eparis@xxxxxxxxxx> wrote:
> On Mon, 2010-11-15 at 00:44 +0000, Alexey Zaytsev wrote:
>> Signed-off-by: Alexey Zaytsev <alexey.zaytsev@xxxxxxxxx>
>> ---
>>
>> Hi.
>>
>> The patch adds fschange-like [1] 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.
>>
>> Looking forwar for your coments on the approach.
>>
>> You can also get the patch 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
>>
>> [1] http://www.stefan.buettcher.org/cs/fschange/index.html
>
> Lets start off by saying I think you need to break this into 3 distinct
> patches.
>
> 1) VFS changes and minimal changes to expose this information to
> fsnotify. ÂWe are going to need VFS/mm people to review that patch and
> don't want them to have to suffer through seeing more changes to
> fs/notify/* than they have to. ÂI don't feel I'm competent to review the
> completeness of this part of the changes...
>
> 2) fsnotify changes which expose the new information to listeners.
>
> 3) fanotify changes which expose the new information to fanotify
> userspace.
>
> Yes, I'm going to probably the only one to review and comment on #2 and
> #3 but it's easier to look at each part of the problem one step at a
> time.

Ack, this was just an rfc, so I thought a single patch would be better.

>> Âfs/nfsd/vfs.c Â Â Â Â Â Â Â Â Â Â Â| Â Â2 +
>> Âfs/notify/fanotify/fanotify_user.c | Â 36 +++++++++++++++++++++--
>> Âfs/notify/fsnotify.c Â Â Â Â Â Â Â | Â 16 ++++++----
>> Âfs/notify/inode_mark.c Â Â Â Â Â Â | Â Â2 +
>> Âfs/notify/inotify/inotify_user.c Â | Â Â2 +
>> Âfs/notify/notification.c Â Â Â Â Â | Â Â8 ++++-
>> Âfs/open.c Â Â Â Â Â Â Â Â Â Â Â Â Â| Â Â2 +
>> Âfs/read_write.c Â Â Â Â Â Â Â Â Â Â| Â Â4 +--
>> Âinclude/linux/fanotify.h Â Â Â Â Â | Â 15 +++++++++-
>> Âinclude/linux/fs.h Â Â Â Â Â Â Â Â | Â 10 ++++++
>> Âinclude/linux/fsnotify.h Â Â Â Â Â | Â 56 ++++++++++++++++++++++++++----------
>> Âinclude/linux/fsnotify_backend.h Â | Â 12 ++++++--
>> Â12 files changed, 128 insertions(+), 37 deletions(-)
>>
>> 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/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index 0632248..5d75dfa 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -31,6 +31,26 @@ struct fanotify_response_event {
>> Â Â Â struct fsnotify_event *event;
>> Â};
>>
>> +#ifdef DEBUG
>> +static inline void dump_event_meta(const char *str, struct fanotify_event_metadata *meta)
>> +{
>> + Â Â if (meta->mask & (FAN_MODIFY | FAN_CLOSE_WRITE))
>> + Â Â Â Â Â Â printk("%s event_len=%d vers=%d mask=0x%lld fd=%d pid=%d"
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â " range_start=%lld range_end=%lld\n", str,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â meta->event_len, meta->vers, meta->mask,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â meta->fd, meta->pid, meta->range.start,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â meta->range.end);
>> + Â Â else
>> + Â Â Â Â Â Â printk("%s event_len=%d vers=%d mask=0x%lld fd=%d pid=%d", str,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â meta->event_len, meta->vers, meta->mask,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â meta->fd, meta->pid);
>> +}
>> +#else
>> +static inline void dump_event_meta(const char *str, struct fanotify_event_metadata *meta)
>> +{
>> +}
>> +#endif
>
> printk() without a KERN_* is wrong. ÂYou also missed the \n in the else
> case. ÂI've been using pr_debug() so I can use dynamic debug rather than
> having to rebuild kernels in the fsnotify code and would love to know if
> we can continue to do that...

Ack.

>> +
>> Â/*
>> Â * Get an fsnotify notification event if one exists and is small
>> Â * enough to fit in "count". Return an error pointer if the count
>> @@ -113,12 +133,20 @@ static ssize_t fill_event_metadata(struct fsnotify_group *group,
>> Â Â Â pr_debug("%s: group=%p metadata=%p event=%p\n", __func__,
>> Â Â Â Â Â Â Â Âgroup, metadata, event);
>>
>> - Â Â metadata->event_len = FAN_EVENT_METADATA_LEN;
>> + Â Â if (event->mask & (FS_MODIFY | FS_CLOSE_WRITE)) {
>> + Â Â Â Â Â Â metadata->event_len = FAN_RANGE_EVENT_METADATA_LEN;
>> + Â Â Â Â Â Â metadata->range.start = event->range.start;
>> + Â Â Â Â Â Â metadata->range.end = event->range.end;
>> + Â Â } else {
>> + Â Â Â Â Â Â 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);
>>
>> +
>
> Extra line.

Ack.
>
>> Â Â Â return metadata->fd;
>> Â}
>>
>> @@ -266,10 +294,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_metadata, fanotify_event_metadata.event_len))
>
> This should be done no matter what....

You mean the original implementation should have done this?

>
>> Â Â Â Â Â Â Â goto out_kill_access_response;
>>
>> - Â Â return FAN_EVENT_METADATA_LEN;
>> + Â Â dump_event_meta("copy_event_to_user: ", &fanotify_event_metadata);
>
> I'd probably just make a single pr_debug() before the copy_to_user and
> expose start/end = -1 if it isn't a modify event....

Ok.

>
>> +
>> + Â Â return fanotify_event_metadata.event_len;
>>
>> Âout_kill_access_response:
>> Â Â Â remove_access_response(group, event, fd);
>> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
>> index 20dc218..81444c2 100644
>> --- a/fs/notify/fsnotify.c
>> +++ b/fs/notify/fsnotify.c
>> @@ -108,10 +108,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, NULL);
>> Â Â Â Â Â Â Â else
>> Â Â Â Â Â Â Â Â Â Â Â ret = fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE,
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdentry->d_name.name, 0);
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdentry->d_name.name, 0, NULL);
>> Â Â Â }
>>
>> Â Â Â dput(parent);
>> @@ -126,6 +126,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;
>> @@ -183,7 +184,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 +198,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 +258,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..29b989f 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;
>>
>> @@ -421,6 +422,9 @@ struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask,
>> Â Â Â event->sync_cookie = cookie;
>> Â Â Â event->to_tell = to_tell;
>> Â Â Â event->data_type = data_type;
>> + Â Â /* The range struct might be allocated on stack. */
>> + Â Â if (range)
>> + Â Â Â Â Â Â event->range = *range;
>
> /me is a bigger fan of memcpy

Maybe gcc would be able to optimize the copy to not involve a call to
memcpy? It's just 128 bytes.

>
>>
>> Â Â Â switch (data_type) {
>> Â Â Â case FSNOTIFY_EVENT_PATH: {
>> @@ -453,7 +457,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/fs/open.c b/fs/open.c
>> index 4197b9e..b3c5b0a 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -675,6 +675,8 @@ 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);
>> + Â Â f->f_whatchanged.start = -1;
>> + Â Â f->f_whatchanged.end = 0;
>> Â Â Â 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 431a0ed..913915d 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -384,7 +384,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);
>> @@ -700,7 +700,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/fanotify.h b/include/linux/fanotify.h
>> index 0f01214..a599517 100644
>> --- a/include/linux/fanotify.h
>> +++ b/include/linux/fanotify.h
>> @@ -91,6 +91,14 @@ struct fanotify_event_metadata {
>> Â Â Â __aligned_u64 mask;
>> Â Â Â __s32 fd;
>> Â Â Â __s32 pid;
>> +
>> + Â Â /* Optional. Check event_len.*/
>> + Â Â union {
>> + Â Â Â Â Â Â struct {
>> + Â Â Â Â Â Â Â Â Â Â __aligned_u64 start;
>> + Â Â Â Â Â Â Â Â Â Â __aligned_u64 end;
>> + Â Â Â Â Â Â } range;
>> + Â Â };
>> Â};
>
> Tvrtko pointed this out, but this is not acceptable as is. ÂAt the VERY
> least it needs a version bump. ÂI admit when I was originally
> considering future expansion of the message type I was assuming that all
> (or almost all) message types would need the same information. ÂYou're
> pointing out that's a bad assumption since we might want to include new
> fields just for 2 message types. ÂThus far it hasn't mattered but I
> guess we are at the decision point.
>
> We've got 3 choices.
>
> 1) Send start/end with every message.
> 2) Make the userspace application know that for MODIFY events of version
> 'X' there will be a range struct right after the pid. ÂIn which case I
> suggest we make an fanotify_modify_event_metadata struct rather than
> unioning into fanotify_event_metadata...
> 3) Do a netlink like expansion semantic, which means you really need a
> next_field_type and next_field_len before the range struct.
>
> I'm also thinking about how we are going to handle the next thing that
> comes up. ÂWhere do we put sender's uid for all message types when
> people ask for that? ÂWe can't put it in front of your new struct (or we
> lose backwards compat). ÂThis is why I want your userspace interface
> propsal to be a separate patch. ÂSo I can more easily concentrate on
> just that one part Â :)

I've proposed a different approach in my reply to Tvrtko, but it's
also wrong, as I've missed the event merges completely. We probably
need to go with 3.
>
> I guess I don't have an answer yet, but you MUST increment the version
> no matter what...
Ack.

>
>> Âstruct fanotify_response {
>> @@ -102,8 +110,13 @@ struct fanotify_response {
>> Â#define FAN_ALLOW Â Â0x01
>> Â#define FAN_DENY Â Â 0x02
>>
>> +#ifndef __KERNEL__
>> +#include <stddef.h> /* For the userspace offsetof */
>> +#endif
>> +
>> Â/* Helper functions to deal with fanotify_event_metadata buffers */
>> -#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))
>> +#define FAN_EVENT_METADATA_LEN (offsetof(struct fanotify_event_metadata, range))
>> +#define FAN_RANGE_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))
>
> I'm questioning if these should be userspace exposed at all or if I need
> to do some better job of FAN_EVENT_OK somehow? ÂNot sure....

The users probably want a FAN_MAX_EVENT_METADATA_LEN, but this creates
backwards-compatibility problems if we expand the event in the future.

>
>> Â#define FAN_EVENT_NEXT(meta, len) ((len) -= (meta)->event_len, \
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(struct fanotify_event_metadata*)(((char *)(meta)) + \
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 334d68a..702d360 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -922,6 +922,12 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
>> Â Â Â Â Â Â Â index < Âra->start + ra->size);
>> Â}
>>
>> +/* fsnotify wants to know, what has been changed during the file's lifetime. */
>> +struct fsnotify_range {
>> + Â Â loff_t start;
>> + Â Â loff_t end;
>> +};
>> +
>> Â#define FILE_MNT_WRITE_TAKEN 1
>> Â#define FILE_MNT_WRITE_RELEASED Â Â Â2
>>
>> @@ -965,6 +971,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..5c5cbaa 100644
>> --- a/include/linux/fsnotify.h
>> +++ b/include/linux/fsnotify.h
>> @@ -57,7 +57,8 @@ static inline int fsnotify_perm(struct file *file, int mask)
>> Â Â Â 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 +79,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 +103,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);
>> Â}
>>
>> @@ -147,7 +151,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 +162,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 +176,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 +190,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);
>> Â}
>>
>> Â/*
>> @@ -201,25 +208,41 @@ static inline void fsnotify_access(struct file *file)
>>
>> Â Â Â if (!(file->f_mode & FMODE_NONOTIFY)) {
>> Â Â Â Â Â Â Â fsnotify_parent(path, NULL, mask);
>> - Â Â Â Â Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
>> + Â Â Â Â Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, NULL);
>> Â Â Â }
>> Â}
>>
>> +static inline void fsnotify_update_range(struct file *f, loff_t orig_fpos, size_t len)
>> +{
>> + Â Â /* -1 => first modification. */
>> + Â Â if (f->f_whatchanged.start == -1)
>> + Â Â Â Â Â Â f->f_whatchanged.start = orig_fpos;
>> + Â Â else
>> + Â Â Â Â Â Â f->f_whatchanged.start = min(orig_fpos, f->f_whatchanged.start);
>
> Would this be faster as:
> Â Â Â Â Â Â Â Âf->f_whatchanged.start = min((unsigned long long)orig_fpos,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (unsigned long long)f->f_whatchanged.start))
>
> (which I think Tvrtko ask)

Hmm, you are right, did not think about it.
>
>> +
>> + Â Â f->f_whatchanged.end = max(orig_fpos + len, f->f_whatchanged.start);
>> +}
>> +
>> Â/*
>> Â * 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, original, count);
>
> I think I'd rather have the helper open coded right here. ÂIt's not
> called anywhere else it is?

Ack.

>
>> Â Â Â 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(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, &range);
>> Â Â Â }
>> Â}
>>
>> @@ -239,7 +262,7 @@ static inline void fsnotify_open(struct file *file)
>> Â Â Â file->f_mode &= ~FMODE_NONOTIFY;
>>
>> Â Â Â fsnotify_parent(path, NULL, mask);
>> - Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
>> + Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0, NULL);
>> Â}
>>
>> Â/*
>> @@ -257,7 +280,8 @@ static inline void fsnotify_close(struct file *file)
>>
>> Â Â Â if (!(file->f_mode & FMODE_NONOTIFY)) {
>> Â Â Â Â Â Â Â fsnotify_parent(path, NULL, mask);
>> - Â Â Â Â Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
>> + Â Â Â Â Â Â fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â NULL, 0, &file->f_whatchanged);
>> Â Â Â }
>> Â}
>>
>> @@ -273,7 +297,7 @@ static inline void fsnotify_xattr(struct dentry *dentry)
>> Â Â Â Â Â Â Â mask |= FS_ISDIR;
>>
>> Â Â Â fsnotify_parent(NULL, dentry, mask);
>> - Â Â fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
>> + Â Â fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
>> Â}
>>
>> Â/*
>> @@ -308,7 +332,7 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
>> Â Â Â Â Â Â Â Â Â Â Â mask |= FS_ISDIR;
>>
>> Â Â Â Â Â Â Â fsnotify_parent(NULL, dentry, mask);
>> - Â Â Â Â Â Â fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
>> + Â Â Â Â Â Â fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0, NULL);
>> Â Â Â }
>> Â}
>>
>> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
>> index 0a68f92..5348b54 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -240,6 +240,8 @@ struct fsnotify_event {
>> Â Â Â size_t name_len;
>> Â Â Â struct pid *tgid;
>>
>> + Â Â struct fsnotify_range range; /* What has been modified */
>> +
>> Â#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>> Â Â Â __u32 response; /* userspace answer to question */
>> Â#endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
>> @@ -305,7 +307,8 @@ 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);
>> + Â Â Â Â Â Â Â Â const unsigned char *name, u32 cookie,
>> + Â Â Â Â Â Â Â Â struct fsnotify_range *range);
>> Âextern int __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask);
>> Âextern void __fsnotify_inode_delete(struct inode *inode);
>> Âextern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
>> @@ -420,7 +423,9 @@ 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);
>> @@ -430,7 +435,8 @@ extern int fsnotify_replace_event(struct fsnotify_event_holder *old_holder,
>> Â#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;
>> Â}
>
> At least those are my first impressions.....
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: James Morris: "Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n"
Previous message: James Morris: "Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n"
In reply to: Eric Paris: "Re: [PATCH] [RFC] fsnotify: Tell the user what part of the filemight have changed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]