[Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
For now, we use inotify watches to track only a small number of
events, namely, add, delete and modify. Note that for delete, the kernel
already deactivates the watch for us and we just need to
take care of modifying our internal state.
Suggested-by: Gerd Hoffman
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 250 ++-
trace-events | 3 +
2 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 37dfa13..79d4ab0 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -15,9 +15,11 @@
#include
#include
+#include
#include "qemu-common.h"
#include "qemu/iov.h"
+#include "qemu/main-loop.h"
#include "trace.h"
#include "hw/usb.h"
#include "hw/usb/desc.h"
@@ -77,6 +79,7 @@ typedef struct MTPState MTPState;
typedef struct MTPControl MTPControl;
typedef struct MTPData MTPData;
typedef struct MTPObject MTPObject;
+typedef struct MTPMonEntry MTPMonEntry;
enum {
EP_DATA_IN = 1,
@@ -84,6 +87,19 @@ enum {
EP_EVENT,
};
+struct MTPMonEntry {
+uint32_t event;
+uint32_t handle;
+
+QTAILQ_ENTRY(MTPMonEntry) next;
+};
+
+enum inotify_event_type {
+CREATE = 1,
+DELETE = 2,
+MODIFY = 3,
+};
+
struct MTPControl {
uint16_t code;
uint32_t trans;
@@ -108,6 +124,8 @@ struct MTPObject {
char *name;
char *path;
struct stat stat;
+/* inotify watch cookie */
+int watchfd;
MTPObject*parent;
uint32_t nchildren;
QLIST_HEAD(, MTPObject) children;
@@ -121,6 +139,8 @@ struct MTPState {
char *root;
char *desc;
uint32_t flags;
+/* inotify descriptor */
+int inotifyfd;
MTPData *data_in;
MTPData *data_out;
@@ -129,6 +149,7 @@ struct MTPState {
uint32_t next_handle;
QTAILQ_HEAD(, MTPObject) objects;
+QTAILQ_HEAD(events, MTPMonEntry) events;
};
#define TYPE_USB_MTP "usb-mtp"
@@ -270,6 +291,40 @@ static const USBDesc desc = {
};
/* --- */
+static MTPObject *usb_mtp_object_lookup_name(MTPObject *parent,
+ char *name, int len)
+{
+MTPObject *iter;
+
+QLIST_FOREACH(iter, &parent->children, list) {
+if (strncmp(iter->name, name, len) == 0) {
+return iter;
+}
+}
+
+return NULL;
+}
+
+static MTPObject *usb_mtp_object_lookup_wd(MTPState *s, int wd)
+{
+MTPObject *iter;
+
+QTAILQ_FOREACH(iter, &s->objects, next) {
+if (iter->watchfd == wd) {
+return iter;
+}
+}
+
+return NULL;
+}
+
+static int usb_mtp_add_watch(int inotifyfd, char *path)
+{
+uint32_t mask = IN_CREATE | IN_DELETE | IN_MODIFY |
+IN_ISDIR;
+
+return inotify_add_watch(inotifyfd, path, mask);
+}
static MTPObject *usb_mtp_object_alloc(MTPState *s, uint32_t handle,
MTPObject *parent, char *name)
@@ -316,6 +371,46 @@ ignore:
return NULL;
}
+static void usb_mtp_inotify_cleanup(MTPState *s)
+{
+MTPMonEntry *e;
+
+if (!s->inotifyfd) {
+return;
+}
+
+qemu_set_fd_handler(s->inotifyfd, NULL, NULL, s);
+close(s->inotifyfd);
+
+QTAILQ_FOREACH(e, &s->events, next) {
+QTAILQ_REMOVE(&s->events, e, next);
+g_free(e);
+}
+}
+
+static int usb_mtp_inotify_mon(MTPState *s, MTPObject *o)
+{
+int watchfd;
+
+if (!s->inotifyfd) {
+return 0;
+}
+assert(o->format == FMT_ASSOCIATION);
+if (o->watchfd > 0) {
+/* already watching */
+return 0;
+}
+
+watchfd = usb_mtp_add_watch(s->inotifyfd, o->path);
+if (watchfd == -1) {
+return 1;
+}
+
+o->watchfd = watchfd;
+trace_usb_mtp_mon(s->dev.addr, o->path, watchfd);
+
+return 0;
+}
static void usb_mtp_object_free(MTPState *s, MTPObject *o)
{
MTPObject *iter;
@@ -370,6 +465,149 @@ static MTPObject *usb_mtp_add_child(MTPState *s,
MTPObject *o,
return child;
}
+static void inotify_watchfn(void *arg)
+{
+MTPState *s = arg;
+ssize_t bytes;
+int error = 0;
+/* From the man page: atleast one event can be read */
+int len = sizeof(struct inotify_event) + NAME_MAX + 1;
+char buf[len];
+
+for (;;) {
+char *p;
+bytes = read(s->inotifyfd, buf, len);
+
+if (bytes <= 0) {
+/* Better luck next time */
+goto done;
+}
+
+/*
+ * TODO: Ignore initiator initiated events.
+ * For now we are good because the store is RO
+ */
+for (p = buf; p < buf + bytes;) {
+struct inotify_event *event = (struct inotify_event *)p;
+int watchfd = 0;
+uint32_t mask = event->mask & (IN_CREATE | IN_DELETE |
+ IN_MODIFY | IN_IGNORED);
+MTPObject *p
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
> +#include
What happens on non-linux systems?
I guess we need some #ifdefs to not break the build there, or enable mtp
only for CONFIG_LINUX=y instead of CONFIG_POSIX=y.
> +enum inotify_event_type {
> +CREATE = 1,
> +DELETE = 2,
> +MODIFY = 3,
> +};
Patch #3 removes this, I'd suggest to extent "enum mtp_code" with the
event codes here so we don't need this temporary thing.
cheers,
Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> +/* Add a new watch asap so as to not lose events
> */
This comment sounds like there is a race ("asap"). There isn't one,
correct ordering (adding the watch before reading the directory) is
enough to make sure you don't miss anything. You might see create
events for objects already in the tree though, are you prepared to
handle that?
cheers,
Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote: > +case IN_DELETE: > +/* > + * The kernel issues a IN_IGNORED event > + * when a dir containing a watchpoint is > + * deleted > + */ Wrong place for the comment?
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Gerd Hoffmann writes:
>> +#include
>
> What happens on non-linux systems?
>
> I guess we need some #ifdefs to not break the build there, or enable mtp
> only for CONFIG_LINUX=y instead of CONFIG_POSIX=y.
Oh, I had the ifdefs but somehow I confused myself by thinking CONFIG_POSIX
is enough not to compile on non-linux systems. I guess I was only thinking
about Windows. I will add them back.
>> +enum inotify_event_type {
>> +CREATE = 1,
>> +DELETE = 2,
>> +MODIFY = 3,
>> +};
>
> Patch #3 removes this, I'd suggest to extent "enum mtp_code" with the
> event codes here so we don't need this temporary thing.
Ok.
> cheers,
> Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Gerd Hoffmann writes:
> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> +/* Add a new watch asap so as to not lose events
>> */
>
> This comment sounds like there is a race ("asap"). There isn't one,
> correct ordering (adding the watch before reading the directory) is
Hmm, seems like there's still a small window. We may not have even
started processing the event because we are still processing the earlier
ones.
> enough to make sure you don't miss anything. You might see create
> events for objects already in the tree though, are you prepared to
> handle that?
Oh, interesting. Current version will happily add duplicate entries.
I will add a check.
> cheers,
> Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Gerd Hoffmann writes: > On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote: >> +case IN_DELETE: >> +/* >> + * The kernel issues a IN_IGNORED event >> + * when a dir containing a watchpoint is >> + * deleted >> + */ > > Wrong place for the comment? I added this to avoid confusion as to why are we not deleting the watch during a delete event. I will reword the comment.
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Bandan Das writes:
> Gerd Hoffmann writes:
>
>> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>>> +/* Add a new watch asap so as to not lose events
>>> */
>>
>> This comment sounds like there is a race ("asap"). There isn't one,
>> correct ordering (adding the watch before reading the directory) is
>
> Hmm, seems like there's still a small window. We may not have even
> started processing the event because we are still processing the earlier
> ones.
>
>> enough to make sure you don't miss anything. You might see create
>> events for objects already in the tree though, are you prepared to
>> handle that?
>
> Oh, interesting. Current version will happily add duplicate entries.
> I will add a check.
By the way, did you mean this as a duplicate create event ?
I took a quick look at fs/notify/inotify_fsnotify.c:
int inotify_handle_event(...
ret = fsnotify_add_event(group, fsn_event, inotify_merge);
if (ret) {
/* Our event wasn't used in the end. Free it. */
fsnotify_destroy_event(group, fsn_event);
}
So, atleast for consecutive duplicate events, the kernel seems to be doing
some filtering of its own.
>> cheers,
>> Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
On Mo, 2015-11-09 at 18:12 -0500, Bandan Das wrote:
> Gerd Hoffmann writes:
>
> > On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> >> +/* Add a new watch asap so as to not lose events
> >> */
> >
> > This comment sounds like there is a race ("asap"). There isn't one,
> > correct ordering (adding the watch before reading the directory) is
>
> Hmm, seems like there's still a small window. We may not have even
> started processing the event because we are still processing the earlier
> ones.
> > enough to make sure you don't miss anything. You might see create
> > events for objects already in the tree though, are you prepared to
> > handle that?
>
> Oh, interesting. Current version will happily add duplicate entries.
> I will add a check.
I think we are talking about the same thing here.
Things can run in parallel, like this:
process copying a file tree | qemu with usb-mtp
+--
create directory|
| inotify event #1 queued (dir)
| qemu fetches event #1
| qemu adds new inotify watch
copy file into new dir |
| inotify event #2 queued (file)
| qemu reads new directory
| qemu finds the new file
| qemu fetches event #2
So, yes, the kernel can add new inotify events for the new watch before
qemu finished processing the old event (especially before you are done
reading the directory), and if you are hitting that the effect is that
you see a create event for the new file even though you already have it
in the tree.
But it is impossible that you miss the creation of the new file (this is
what I meant with "there is no race").
hope this clarifies,
Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Gerd Hoffmann writes:
> On Mo, 2015-11-09 at 18:12 -0500, Bandan Das wrote:
>> Gerd Hoffmann writes:
>>
>> > On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> >> +/* Add a new watch asap so as to not lose events
>> >> */
>> >
>> > This comment sounds like there is a race ("asap"). There isn't one,
>> > correct ordering (adding the watch before reading the directory) is
>>
>> Hmm, seems like there's still a small window. We may not have even
>> started processing the event because we are still processing the earlier
>> ones.
>
>> > enough to make sure you don't miss anything. You might see create
>> > events for objects already in the tree though, are you prepared to
>> > handle that?
>>
>> Oh, interesting. Current version will happily add duplicate entries.
>> I will add a check.
>
> I think we are talking about the same thing here.
> Things can run in parallel, like this:
>
> process copying a file tree | qemu with usb-mtp
> +--
> create directory|
> | inotify event #1 queued (dir)
> | qemu fetches event #1
> | qemu adds new inotify watch
> copy file into new dir |
> | inotify event #2 queued (file)
> | qemu reads new directory
> | qemu finds the new file
> | qemu fetches event #2
>
> So, yes, the kernel can add new inotify events for the new watch before
Maybe I am missing something but what if the watch on dir was
added by qemu _after_ the file (say file1) was copied to it.
Then, the kernel would generate events for file2, file3 and so on but
never a CREATE event for file1. Isn't that a possibility ? So, what I mean
by that comment is that add a watchpoint soon enough but it could be
possible that by the time the watch is added, a few files might have already
been copied and will not generate events.
> qemu finished processing the old event (especially before you are done
> reading the directory), and if you are hitting that the effect is that
> you see a create event for the new file even though you already have it
> in the tree.
>
> But it is impossible that you miss the creation of the new file (this is
> what I meant with "there is no race").
>
> hope this clarifies,
> Gerd
Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring
Hi, > Maybe I am missing something but what if the watch on dir was > added by qemu _after_ the file (say file1) was copied to it. > Then, the kernel would generate events for file2, file3 and so on but > never a CREATE event for file1. Isn't that a possibility ? Yes. > So, what I mean > by that comment is that add a watchpoint soon enough but it could be > possible that by the time the watch is added, a few files might have already > been copied and will not generate events. The readdir (in usb_mtp_object_readdir) will find them then. While looking at the code: I think there is no need to add the watch right away. We only read the directory when the guest actually requests it. Watching the directories we actually have scanned due to the guest requesting it should be enough. So I think adding the inotify watch in usb_mtp_object_readdir should work just fine. And the watch should be added before readdir to make sure we don't miss anything. File system events then can happen at three points in time: (a) before we add the watch, (b) after we add the watch, and before readdir, and (c) after readdir. (a) we will see these files in the readdir call. (b) we will see these files in the readdir call *and* receive inotify events for them. (c) we will only get events for them. The (b) events look bogous at first glance (create events for files you already have in the list, but also delete events for files you don't have in your list). cheers, Gerd
