This patch adds the 'waiting' attribute which indicates how many
filesystem requests are currently waiting to be completed. A non-zero
value without any filesystem activity indicates a hung or deadlocked
filesystem.
Signed-off-by: Miklos Szeredi <[email protected]>
Index: linux/fs/fuse/inode.c
===================================================================
--- linux.orig/fs/fuse/inode.c 2006-01-14 00:22:44.000000000 +0100
+++ linux/fs/fuse/inode.c 2006-01-14 00:33:16.000000000 +0100
@@ -555,7 +555,16 @@ static struct file_system_type fuse_fs_t
.kill_sb = kill_anon_super,
};
+static ssize_t fuse_conn_waiting_show(struct fuse_conn *fc, char *page)
+{
+ return sprintf(page, "%i\n", atomic_read(&fc->num_waiting));
+}
+
+static struct fuse_conn_attr fuse_conn_waiting =
+ __ATTR(waiting, 0400, fuse_conn_waiting_show, NULL);
+
static struct attribute *fuse_conn_attrs[] = {
+ &fuse_conn_waiting.attr,
NULL,
};
Index: linux/fs/fuse/dev.c
===================================================================
--- linux.orig/fs/fuse/dev.c 2006-01-14 00:22:44.000000000 +0100
+++ linux/fs/fuse/dev.c 2006-01-14 00:23:16.000000000 +0100
@@ -109,18 +109,24 @@ struct fuse_req *fuse_get_request(struct
int intr;
sigset_t oldset;
+ atomic_inc(&fc->num_waiting);
block_sigs(&oldset);
intr = down_interruptible(&fc->outstanding_sem);
restore_sigs(&oldset);
- return intr ? NULL : do_get_request(fc);
+ if (intr) {
+ atomic_dec(&fc->num_waiting);
+ return NULL;
+ }
+ return do_get_request(fc);
}
static void fuse_putback_request(struct fuse_conn *fc, struct fuse_req *req)
{
spin_lock(&fuse_lock);
- if (req->preallocated)
+ if (req->preallocated) {
+ atomic_dec(&fc->num_waiting);
list_add(&req->list, &fc->unused_list);
- else
+ } else
fuse_request_free(req);
/* If we are in debt decrease that first */
Index: linux/fs/fuse/fuse_i.h
===================================================================
--- linux.orig/fs/fuse/fuse_i.h 2006-01-14 00:22:44.000000000 +0100
+++ linux/fs/fuse/fuse_i.h 2006-01-14 00:23:16.000000000 +0100
@@ -280,6 +280,9 @@ struct fuse_conn {
/** Is create not implemented by fs? */
unsigned no_create : 1;
+ /** The number of requests waiting for completion */
+ atomic_t num_waiting;
+
/** Negotiated minor version */
unsigned minor;
--
Miklos Szeredi <[email protected]> wrote:
>
> + /** The number of requests waiting for completion */
> + atomic_t num_waiting;
This doesn't get initialised anywhere.
Presumably you're relying on a memset somewhere. That might work on all
architectures, AFAIK. But in theory it's wrong. If, for example, the
architecture implements atomic_t via a spinlock-plus-integer, and that
spinlock's unlocked state is not all-bits-zero, we're dead.
So we should initialise it with
foo->num_waiting = ATOMIC_INIT(0);
nb: it is not correct to initialise an atomic_t with
atomic_set(a, 0);
because in the above theoretical case case where the arch uses a spinlock
in the atomic_t, that spinlock doesn't get initialised. I bet we've got code
in there which does this.
> This doesn't get initialised anywhere.
>
> Presumably you're relying on a memset somewhere. That might work on all
> architectures, AFAIK. But in theory it's wrong. If, for example, the
> architecture implements atomic_t via a spinlock-plus-integer, and that
> spinlock's unlocked state is not all-bits-zero, we're dead.
>
> So we should initialise it with
>
> foo->num_waiting = ATOMIC_INIT(0);
>
Is it correct to use a structure initializer this way?
> nb: it is not correct to initialise an atomic_t with
>
> atomic_set(a, 0);
>
> because in the above theoretical case case where the arch uses a spinlock
> in the atomic_t, that spinlock doesn't get initialised. I bet we've got code
> in there which does this.
According to Documentation/atomic_ops.txt, this is the correct usage
of atomic_set():
| The first operations to implement for atomic_t's are the
| initializers and plain reads.
|
| #define ATOMIC_INIT(i) { (i) }
| #define atomic_set(v, i) ((v)->counter = (i))
|
| The first macro is used in definitions, such as:
|
| static atomic_t my_counter = ATOMIC_INIT(1);
|
| The second interface can be used at runtime, as in:
|
| struct foo { atomic_t counter; };
| ...
|
| struct foo *k;
|
| k = kmalloc(sizeof(*k), GFP_KERNEL);
| if (!k)
| return -ENOMEM;
| atomic_set(&k->counter, 0);
So in fact atomic_set() is an initializer, and should be named
atomic_init() accordingly. Is atomic_set() ever used as an atomic
operation rather than an initializer?
Miklos
Miklos Szeredi <[email protected]> wrote:
>
> > This doesn't get initialised anywhere.
> >
> > Presumably you're relying on a memset somewhere. That might work on all
> > architectures, AFAIK. But in theory it's wrong. If, for example, the
> > architecture implements atomic_t via a spinlock-plus-integer, and that
> > spinlock's unlocked state is not all-bits-zero, we're dead.
> >
> > So we should initialise it with
> >
> > foo->num_waiting = ATOMIC_INIT(0);
> >
>
> Is it correct to use a structure initializer this way?
Yes, if it's typecast to the right type.
ATOMIC_INIT is not. I had a brainfart.
> > nb: it is not correct to initialise an atomic_t with
> >
> > atomic_set(a, 0);
> >
> > because in the above theoretical case case where the arch uses a spinlock
> > in the atomic_t, that spinlock doesn't get initialised. I bet we've got code
> > in there which does this.
>
> According to Documentation/atomic_ops.txt, this is the correct usage
> of atomic_set():
>
> | The first operations to implement for atomic_t's are the
> | initializers and plain reads.
> |
> | #define ATOMIC_INIT(i) { (i) }
> | #define atomic_set(v, i) ((v)->counter = (i))
> |
> | The first macro is used in definitions, such as:
> |
> | static atomic_t my_counter = ATOMIC_INIT(1);
> |
> | The second interface can be used at runtime, as in:
> |
> | struct foo { atomic_t counter; };
> | ...
> |
> | struct foo *k;
> |
> | k = kmalloc(sizeof(*k), GFP_KERNEL);
> | if (!k)
> | return -ENOMEM;
> | atomic_set(&k->counter, 0);
>
> So in fact atomic_set() is an initializer, and should be named
> atomic_init() accordingly.
Yes, we're screwed. I don't think it's possible to implement atomic_t as
spinlock+int due to this.
> Is atomic_set() ever used as an atomic
> operation rather than an initializer?
>
Sure, lots of places. Lots of places where you _don't_ want your
atomic_t's spinlock to be reinitialised.
hmm.
Andrew Morton a ?crit :
> Miklos Szeredi <[email protected]> wrote:
>> + /** The number of requests waiting for completion */
>> + atomic_t num_waiting;
>
> This doesn't get initialised anywhere.
>
> Presumably you're relying on a memset somewhere. That might work on all
> architectures, AFAIK. But in theory it's wrong. If, for example, the
> architecture implements atomic_t via a spinlock-plus-integer, and that
> spinlock's unlocked state is not all-bits-zero, we're dead.
>
> So we should initialise it with
>
> foo->num_waiting = ATOMIC_INIT(0);
>
>
>
> nb: it is not correct to initialise an atomic_t with
>
> atomic_set(a, 0);
>
> because in the above theoretical case case where the arch uses a spinlock
> in the atomic_t, that spinlock doesn't get initialised. I bet we've got code
> in there which does this.
Hum... I tracked one missing atomic_set() or ATOMIC_INIT in e1000 driver then.
e1000_alloc_queues() does :
#ifdef CONFIG_E1000_NAPI
size = sizeof(struct net_device) * adapter->num_queues;
adapter->polling_netdev = kmalloc(size, GFP_KERNEL);
if (!adapter->polling_netdev) {
kfree(adapter->tx_ring);
kfree(adapter->rx_ring);
return -ENOMEM;
}
memset(adapter->polling_netdev, 0, size);
#endif
So this driver clearly assumes a memset(... 0 ...) also initialize atomic_t to
0 ((struct net_device *)->refcnt for example)
Eric