[PATCH] relay: move remove_buf_file inside relay_close_buf

[PATCH] relay: move remove_buf_file inside relay_close_buf

[Dmitry Monakhov]

Currently remove_buf_file callback is called from from kobject
release method. This result in follow issue:
# blktrace -d /dev/sda1 -d /dev/sda -o test
blktrace_setup()
 dir = create_dir()
 rchan = relay_open(dir,...)
 ->create_buf_file_callback
    buf_file  = debugfs_create_file(dir, )

Userspace will open buf_file.
Later we make a decision to stop tracing
blktrace_down()
  relay_close(rhcan)  /* just decrement kobj reference  */
                      /* since it is not zero then callback not called */
  debugfs_remove(dir) /* FAIL due to non empty dir   */

Later user space will close the file and file will be deleted,
but directory still exist.
user_space_close()
 ->file_release
   ->release_buf_file_callback
     ->debugfs_remove(buf_file)

In fact this is general issue, blktrace is just one of examples.
We can not reliably remove parent dir until all users close the
buf_file.

We don't have to wait this long. File may be deleted
inside relay_close_buf().

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 kernel/relay.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/relay.c b/kernel/relay.c
index c705a41..dcf71c8 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -237,7 +237,6 @@ static void relay_destroy_buf(struct rchan_buf *buf)
 static void relay_remove_buf(struct kref *kref)
 {
 	struct rchan_buf *buf = container_of(kref, struct rchan_buf, kref);
-	buf->chan->cb->remove_buf_file(buf->dentry);
 	relay_destroy_buf(buf);
 }
 
@@ -487,6 +486,7 @@ static void relay_close_buf(struct rchan_buf *buf)
 {
 	buf->finalized = 1;
 	del_timer_sync(&buf->timer);
+	buf->chan->cb->remove_buf_file(buf->dentry);
 	kref_put(&buf->kref, relay_remove_buf);
 }
 
-- 
1.6.6

Re: [PATCH] relay: move remove_buf_file inside relay_close_buf

[Jens Axboe]

On Sat, Feb 27 2010, Dmitry Monakhov wrote:
> Currently remove_buf_file callback is called from from kobject
> release method. This result in follow issue:
> # blktrace -d /dev/sda1 -d /dev/sda -o test
> blktrace_setup()
>  dir = create_dir()
>  rchan = relay_open(dir,...)
>  ->create_buf_file_callback
>     buf_file  = debugfs_create_file(dir, )
> 
> Userspace will open buf_file.
> Later we make a decision to stop tracing
> blktrace_down()
>   relay_close(rhcan)  /* just decrement kobj reference  */
>                       /* since it is not zero then callback not called */
>   debugfs_remove(dir) /* FAIL due to non empty dir   */
> 
> Later user space will close the file and file will be deleted,
> but directory still exist.
> user_space_close()
>  ->file_release
>    ->release_buf_file_callback
>      ->debugfs_remove(buf_file)
> 
> In fact this is general issue, blktrace is just one of examples.
> We can not reliably remove parent dir until all users close the
> buf_file.
> 
> We don't have to wait this long. File may be deleted
> inside relay_close_buf().

Thanks, I believe this fixes a long standing problem we've had with
blktrace. You can add my acked-by.

-- 
Jens Axboe

Re: [PATCH] relay: move remove_buf_file inside relay_close_buf

[Dmitry Monakhov]

add Tom Zanussi to recipients.
Tom, please take a look at the patch, it is simple and clean and it able
to solve long standing blktrace related bug.
Dmitry Monakhov <dmonakhov@openvz.org> writes:
> Currently remove_buf_file callback is called from from kobject
> release method. This result in follow issue:
> # blktrace -d /dev/sda1 -d /dev/sda -o test
> blktrace_setup()
>  dir = create_dir()
>  rchan = relay_open(dir,...)
>  ->create_buf_file_callback
>     buf_file  = debugfs_create_file(dir, )
>
> Userspace will open buf_file.
> Later we make a decision to stop tracing
> blktrace_down()
>   relay_close(rhcan)  /* just decrement kobj reference  */
>                       /* since it is not zero then callback not called */
>   debugfs_remove(dir) /* FAIL due to non empty dir   */
>
> Later user space will close the file and file will be deleted,
> but directory still exist.
> user_space_close()
>  ->file_release
>    ->release_buf_file_callback
>      ->debugfs_remove(buf_file)
>
> In fact this is general issue, blktrace is just one of examples.
> We can not reliably remove parent dir until all users close the
> buf_file.
>
> We don't have to wait this long. File may be deleted
> inside relay_close_buf().
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  kernel/relay.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/relay.c b/kernel/relay.c
> index c705a41..dcf71c8 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -237,7 +237,6 @@ static void relay_destroy_buf(struct rchan_buf *buf)
>  static void relay_remove_buf(struct kref *kref)
>  {
>  	struct rchan_buf *buf = container_of(kref, struct rchan_buf, kref);
> -	buf->chan->cb->remove_buf_file(buf->dentry);
>  	relay_destroy_buf(buf);
>  }
>  
> @@ -487,6 +486,7 @@ static void relay_close_buf(struct rchan_buf *buf)
>  {
>  	buf->finalized = 1;
>  	del_timer_sync(&buf->timer);
> +	buf->chan->cb->remove_buf_file(buf->dentry);
>  	kref_put(&buf->kref, relay_remove_buf);
>  }

Re: [PATCH] relay: move remove_buf_file inside relay_close_buf

[Tom Zanussi]

On Tue, 2010-04-06 at 13:34 +0400, Dmitry Monakhov wrote:
> add Tom Zanussi to recipients.
> Tom, please take a look at the patch, it is simple and clean and it able
> to solve long standing blktrace related bug.

Hi,

It looks ok to me - I don't see any problems with it, but you should
probably get the official relay maintainer(s?) to take a look at it as
well - adding them to the cc: list.

Tom

> Dmitry Monakhov <dmonakhov@openvz.org> writes:
> > Currently remove_buf_file callback is called from from kobject
> > release method. This result in follow issue:
> > # blktrace -d /dev/sda1 -d /dev/sda -o test
> > blktrace_setup()
> >  dir = create_dir()
> >  rchan = relay_open(dir,...)
> >  ->create_buf_file_callback
> >     buf_file  = debugfs_create_file(dir, )
> >
> > Userspace will open buf_file.
> > Later we make a decision to stop tracing
> > blktrace_down()
> >   relay_close(rhcan)  /* just decrement kobj reference  */
> >                       /* since it is not zero then callback not called */
> >   debugfs_remove(dir) /* FAIL due to non empty dir   */
> >
> > Later user space will close the file and file will be deleted,
> > but directory still exist.
> > user_space_close()
> >  ->file_release
> >    ->release_buf_file_callback
> >      ->debugfs_remove(buf_file)
> >
> > In fact this is general issue, blktrace is just one of examples.
> > We can not reliably remove parent dir until all users close the
> > buf_file.
> >
> > We don't have to wait this long. File may be deleted
> > inside relay_close_buf().
> >
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  kernel/relay.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index c705a41..dcf71c8 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -237,7 +237,6 @@ static void relay_destroy_buf(struct rchan_buf *buf)
> >  static void relay_remove_buf(struct kref *kref)
> >  {
> >  	struct rchan_buf *buf = container_of(kref, struct rchan_buf, kref);
> > -	buf->chan->cb->remove_buf_file(buf->dentry);
> >  	relay_destroy_buf(buf);
> >  }
> >  
> > @@ -487,6 +486,7 @@ static void relay_close_buf(struct rchan_buf *buf)
> >  {
> >  	buf->finalized = 1;
> >  	del_timer_sync(&buf->timer);
> > +	buf->chan->cb->remove_buf_file(buf->dentry);
> >  	kref_put(&buf->kref, relay_remove_buf);
> >  }

[PATCH] relay: move remove_buf_file inside relay_close_buf

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]


Hi, 
Let someone please finally take care of this patch.
Originally it was submitted here https://lkml.org/lkml/2010/2/28/103
This patch fix very annoying issue. I've got OK from Jens and Tom
Zanussi but no one accept it. MAINTAINERS file has no info about relayfs.
So I've added to CC all people which may be interested in that.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-relay-move-remove_buf_file-inside-relay_close_buf.patch --]
[-- Type: text/x-patch, Size: 2188 bytes --]

>From 1c0d96aece60a8a81c3f0cf1f681a5ff4333a2ff Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Mon, 22 Apr 2013 11:41:41 +0400
Subject: [PATCH] relay: move remove_buf_file inside relay_close_buf

Currently remove_buf_file callback is called from from kobject
release method. This result in follow issue:
# blktrace -d /dev/sda1 -d /dev/sda -o test

blktrace_setup()
 dir = create_dir()
 rchan = relay_open(dir,...)
 ->create_buf_file_callback
    buf_file  = debugfs_create_file(dir, )

Userspace will open buf_file.
Later we make a decision to stop tracing
blktrace_down()
  relay_close(rhcan)  /* just decrement kobj reference  */
                      /* since it is not zero then callback not called */
  debugfs_remove(dir) /* FAIL due to non empty dir   */

Later user space will close the file and file will be deleted,
but directory still exist.
user_space_close()
 ->file_release
   ->release_buf_file_callback
     ->debugfs_remove(buf_file
## TESTCASE:
# blktrace -d /dev/sda1 -d /dev/sda -o test
# After that blktrace infrastructure will remain broken in
# an unusable state so: blktrace -d /dev/sda1 will not work.

In fact this is general issue, blktrace is just one of examples.
We can not reliably remove parent dir until all users close the
buf_file.

Solution: We don't have to wait that long. File should be deleted inside
relay_close_buf().

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
Acked-by : Jens Axboe <axboe@kernel.dk>

---
 kernel/relay.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/relay.c b/kernel/relay.c
index 01ab081..a0d2000 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -234,7 +234,6 @@ static void relay_destroy_buf(struct rchan_buf *buf)
 static void relay_remove_buf(struct kref *kref)
 {
 	struct rchan_buf *buf = container_of(kref, struct rchan_buf, kref);
-	buf->chan->cb->remove_buf_file(buf->dentry);
 	relay_destroy_buf(buf);
 }
 
@@ -484,6 +483,7 @@ static void relay_close_buf(struct rchan_buf *buf)
 {
 	buf->finalized = 1;
 	del_timer_sync(&buf->timer);
+	buf->chan->cb->remove_buf_file(buf->dentry);
 	kref_put(&buf->kref, relay_remove_buf);
 }
 
-- 
1.7.1

Re: [PATCH] relay: move remove_buf_file inside relay_close_buf PING..

[Dmitry Monakhov]

On Mon, 22 Apr 2013 11:57:52 +0400, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
Non-text part: multipart/mixed
> 
Seriously. Am I an only one who use blktrace?
Jens can you please take it?
> Hi,
> Let someone please finally take care of this patch.
> Originally it was submitted here https://lkml.org/lkml/2010/2/28/103
> This patch fix very annoying issue. I've got OK from Jens and Tom
> Zanussi but no one accept it. MAINTAINERS file has no info about relayfs.
> So I've added to CC all people which may be interested in that.
> 
> From 1c0d96aece60a8a81c3f0cf1f681a5ff4333a2ff Mon Sep 17 00:00:00 2001
> From: Dmitry Monakhov <dmonakhov@openvz.org>
> Date: Mon, 22 Apr 2013 11:41:41 +0400
> Subject: [PATCH] relay: move remove_buf_file inside relay_close_buf
> 
> Currently remove_buf_file callback is called from from kobject
> release method. This result in follow issue:
> # blktrace -d /dev/sda1 -d /dev/sda -o test
> 
> blktrace_setup()
>  dir = create_dir()
>  rchan = relay_open(dir,...)
>  ->create_buf_file_callback
>     buf_file  = debugfs_create_file(dir, )
> 
> Userspace will open buf_file.
> Later we make a decision to stop tracing
> blktrace_down()
>   relay_close(rhcan)  /* just decrement kobj reference  */
>                       /* since it is not zero then callback not called */
>   debugfs_remove(dir) /* FAIL due to non empty dir   */
> 
> Later user space will close the file and file will be deleted,
> but directory still exist.
> user_space_close()
>  ->file_release
>    ->release_buf_file_callback
>      ->debugfs_remove(buf_file
> ## TESTCASE:
> # blktrace -d /dev/sda1 -d /dev/sda -o test
> # After that blktrace infrastructure will remain broken in
> # an unusable state so: blktrace -d /dev/sda1 will not work.
> 
> In fact this is general issue, blktrace is just one of examples.
> We can not reliably remove parent dir until all users close the
> buf_file.
> 
> Solution: We don't have to wait that long. File should be deleted inside
> relay_close_buf().
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> Acked-by : Jens Axboe <axboe@kernel.dk>
> 
> ---
>  kernel/relay.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 01ab081..a0d2000 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -234,7 +234,6 @@ static void relay_destroy_buf(struct rchan_buf *buf)
>  static void relay_remove_buf(struct kref *kref)
>  {
>  	struct rchan_buf *buf = container_of(kref, struct rchan_buf, kref);
> -	buf->chan->cb->remove_buf_file(buf->dentry);
>  	relay_destroy_buf(buf);
>  }
>  
> @@ -484,6 +483,7 @@ static void relay_close_buf(struct rchan_buf *buf)
>  {
>  	buf->finalized = 1;
>  	del_timer_sync(&buf->timer);
> +	buf->chan->cb->remove_buf_file(buf->dentry);
>  	kref_put(&buf->kref, relay_remove_buf);
>  }
>  
> -- 
> 1.7.1
> 

Re: [PATCH] relay: move remove_buf_file inside relay_close_buf PING..

[Jens Axboe]

On Tue, Apr 30 2013, Dmitry Monakhov wrote:
> On Mon, 22 Apr 2013 11:57:52 +0400, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
> Non-text part: multipart/mixed
> > 
> Seriously. Am I an only one who use blktrace?

I do :-)

> Jens can you please take it?

Thanks, will do.

-- 
Jens Axboe