From: Al Viro <viro@ZenIV.linux.org.uk>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Davide Libenzi <davidel@xmailserver.org>,
Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH] vfio-pci: PCI hot reset interface
Date: Tue, 20 Aug 2013 04:18:21 +0100 [thread overview]
Message-ID: <20130820031821.GF27005@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CAErSpo4zfuaaBSDci4jOu_9RgUfLQNPvKhro1OEh=8AMvAFWYw@mail.gmail.com>
On Wed, Aug 14, 2013 at 04:42:14PM -0600, Bjorn Helgaas wrote:
> [+cc Al, linux-fsdevel for fdget/fdput usage]
fdget/fdput use looks sane, the only thing is that I would rather
have an explicit include of linux/file.h instead of relying upon
linux/eventfd.h pulling it. Incidentally, there are only 5 files
that include the latter without an explicit include of the former -
drivers/vfio/pci/vfio_pci.c, drivers/vhost/scsi.c, kernel/cgroup.c,
mm/memcontrol.c and mm/vmpressure.c. And only kernel/cgroup.c (and,
with this patch, vfio_pci.c) really wants anything from linux/file.h,
so I'd rather kill that indirect include in eventfd.h and slapped
an explicit include of file.h in these two files...
BTW, most of the eventfd_fget() users might as well be using fget()
(or fdget(), for that matter). They tend to be immediately followed
by eventfd_ctx_fileget(), which repeats the "is that an eventfd file?"
check anyway.
Completely untested patch below does that to kernel/cgroup.c; Tejun,
Davide - do you have any objections against the following?
Kill indirect include of file.h from eventfd.h, use fdget() in cgroup.c
kernel/cgroup.c is the only place in the tree that relies on eventfd.h
pulling file.h; move that include there. Switch from eventfd_fget()/fput()
to fdget()/fdput(), while we are at it - eventfd_ctx_fileget() will fail
on non-eventfd descriptors just fine, no need to do that check twice...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index cf5d2af..ff0b981 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -9,7 +9,6 @@
#define _LINUX_EVENTFD_H
#include <linux/fcntl.h>
-#include <linux/file.h>
#include <linux/wait.h>
/*
@@ -26,6 +25,8 @@
#define EFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
#define EFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS | EFD_SEMAPHORE)
+struct file;
+
#ifdef CONFIG_EVENTFD
struct file *eventfd_file_create(unsigned int count, int flags);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 781845a..f88ecaf 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
#include <linux/poll.h>
#include <linux/flex_array.h> /* used in cgroup_attach_task */
#include <linux/kthread.h>
+#include <linux/file.h>
#include <linux/atomic.h>
@@ -3969,8 +3970,8 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
struct cgroup_event *event = NULL;
struct cgroup *cgrp_cfile;
unsigned int efd, cfd;
- struct file *efile = NULL;
- struct file *cfile = NULL;
+ struct fd efile;
+ struct fd cfile;
char *endp;
int ret;
@@ -3993,31 +3994,31 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
init_waitqueue_func_entry(&event->wait, cgroup_event_wake);
INIT_WORK(&event->remove, cgroup_event_remove);
- efile = eventfd_fget(efd);
- if (IS_ERR(efile)) {
- ret = PTR_ERR(efile);
- goto fail;
+ efile = fdget(efd);
+ if (!efile.file) {
+ ret = -EBADF;
+ goto fail1;
}
- event->eventfd = eventfd_ctx_fileget(efile);
+ event->eventfd = eventfd_ctx_fileget(efile.file);
if (IS_ERR(event->eventfd)) {
ret = PTR_ERR(event->eventfd);
- goto fail;
+ goto fail2;
}
- cfile = fget(cfd);
- if (!cfile) {
+ cfile = fdget(cfd);
+ if (!cfile.file) {
ret = -EBADF;
- goto fail;
+ goto fail3;
}
/* the process need read permission on control file */
/* AV: shouldn't we check that it's been opened for read instead? */
- ret = inode_permission(file_inode(cfile), MAY_READ);
+ ret = inode_permission(file_inode(cfile.file), MAY_READ);
if (ret < 0)
goto fail;
- event->cft = __file_cft(cfile);
+ event->cft = __file_cft(cfile.file);
if (IS_ERR(event->cft)) {
ret = PTR_ERR(event->cft);
goto fail;
@@ -4027,7 +4028,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
* The file to be monitored must be in the same cgroup as
* cgroup.event_control is.
*/
- cgrp_cfile = __d_cgrp(cfile->f_dentry->d_parent);
+ cgrp_cfile = __d_cgrp(cfile.file->f_dentry->d_parent);
if (cgrp_cfile != cgrp) {
ret = -EINVAL;
goto fail;
@@ -4043,7 +4044,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
if (ret)
goto fail;
- efile->f_op->poll(efile, &event->pt);
+ efile.file->f_op->poll(efile.file, &event->pt);
/*
* Events should be removed after rmdir of cgroup directory, but before
@@ -4056,21 +4057,18 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
list_add(&event->list, &cgrp->event_list);
spin_unlock(&cgrp->event_list_lock);
- fput(cfile);
- fput(efile);
+ fdput(cfile);
+ fdput(efile);
return 0;
fail:
- if (cfile)
- fput(cfile);
-
- if (event && event->eventfd && !IS_ERR(event->eventfd))
- eventfd_ctx_put(event->eventfd);
-
- if (!IS_ERR_OR_NULL(efile))
- fput(efile);
-
+ fdput(cfile);
+fail3:
+ eventfd_ctx_put(event->eventfd);
+fail2:
+ fdput(efile);
+fail1:
kfree(event);
return ret;
next prev parent reply other threads:[~2013-08-20 3:18 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-14 20:10 [PATCH] vfio-pci: PCI hot reset interface Alex Williamson
2013-08-14 22:42 ` Bjorn Helgaas
2013-08-14 23:06 ` Alex Williamson
2013-08-19 18:41 ` Alex Williamson
2013-08-19 20:02 ` Bjorn Helgaas
2013-08-19 20:20 ` Alex Williamson
2013-08-19 22:44 ` Benjamin Herrenschmidt
2013-08-19 23:02 ` Alex Williamson
2013-08-19 22:42 ` Benjamin Herrenschmidt
2013-08-19 22:59 ` Alex Williamson
2013-08-19 23:52 ` Benjamin Herrenschmidt
2013-08-20 3:18 ` Al Viro [this message]
2013-08-20 3:53 ` Alex Williamson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130820031821.GF27005@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=alex.williamson@redhat.com \
--cc=benh@kernel.crashing.org \
--cc=bhelgaas@google.com \
--cc=davidel@xmailserver.org \
--cc=kvm@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).