Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 3 Apr 2021 17:04:47 +0800
From:   Ming Lei <ming.lei@redhat.com>
To:     Christoph Hellwig <hch@lst.de>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] block: shutdown blktrace in case of fatal signal
 pending
Message-ID: <YGgvrxiQUTQMfUeN@T590>
References: <20210323081440.81343-1-ming.lei@redhat.com>
 <20210323081440.81343-2-ming.lei@redhat.com>
 <20210330165330.GA13829@lst.de>
 <YGO/cpalyGevAJjn@T590>
 <20210402172730.GA22923@lst.de>
 <YGgi6FOr6cEiei+7@T590>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YGgi6FOr6cEiei+7@T590>
Precedence: bulk

On Sat, Apr 03, 2021 at 04:10:16PM +0800, Ming Lei wrote:
> On Fri, Apr 02, 2021 at 07:27:30PM +0200, Christoph Hellwig wrote:
> > On Wed, Mar 31, 2021 at 08:16:50AM +0800, Ming Lei wrote:
> > > On Tue, Mar 30, 2021 at 06:53:30PM +0200, Christoph Hellwig wrote:
> > > > On Tue, Mar 23, 2021 at 04:14:39PM +0800, Ming Lei wrote:
> > > > > blktrace may allocate lots of memory, if the process is terminated
> > > > > by user or OOM, we need to provide one chance to remove the trace
> > > > > buffer, otherwise memory leak may be caused.
> > > > > 
> > > > > Fix the issue by shutdown blktrace in case of task exiting in
> > > > > blkdev_close().
> > > > > 
> > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > 
> > > > This just seems weird.  blktrace has no relationship to open
> > > > block device instances.
> > > 
> > > blktrace still needs to open one blkdev, then send its own ioctl
> > > commands to block layer. In case of OOM, the allocated memory in
> > > these ioctl commands won't be released.
> > > 
> > > Or any other suggestion?
> > 
> > Not much we can do there I think.  If we want to autorelease memory
> > it needs to be an API that ties the memory allocation to an FD.
> 
> We still may shutdown blktrace if current is the last opener, otherwise
> new blktrace can't be started and memory should be leaked forever, and
> what do you think of the revised version?

This way seems not good enough, another better one is to use
file->private_data for such purpose since blkdev fs doesn't use
file->privete_data, then we can shutdown blktrace just for the
blktrace FD:

From 191dff30abfd48c38a78dec78e011a39a3b606ca Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Tue, 23 Mar 2021 10:32:23 +0800
Subject: [PATCH] block: shutdown blktrace in case of task exiting

blktrace may allocate lots of memory, if the process is terminated
by user or OOM, we need to provide one chance to remove the trace
buffer, otherwise memory leak may be caused. Also new blktrace
instance can't be started too.

Fix the issue by shutdown blktrace in bdev_close() if blktrace
was setup on this FD.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/ioctl.c                |  2 ++
 fs/block_dev.c               | 12 ++++++++++++
 include/linux/blktrace_api.h | 11 +++++++++++
 3 files changed, 25 insertions(+)

diff --git a/block/ioctl.c b/block/ioctl.c
index ff241e663c01..7dad4a546db3 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -611,6 +611,8 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 	else
 		mode &= ~FMODE_NDELAY;
 
+	blkdev_mark_blktrace(file, cmd);
+
 	switch (cmd) {
 	/* These need separate implementations for the data structure */
 	case HDIO_GETGEO:
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 92ed7d5df677..aaa7d7d1e5a4 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -34,6 +34,7 @@
 #include <linux/part_stat.h>
 #include <linux/uaccess.h>
 #include <linux/suspend.h>
+#include <linux/blktrace_api.h>
 #include "internal.h"
 
 struct bdev_inode {
@@ -1646,6 +1647,15 @@ EXPORT_SYMBOL(blkdev_put);
 static int blkdev_close(struct inode * inode, struct file * filp)
 {
 	struct block_device *bdev = I_BDEV(bdev_file_inode(filp));
+
+	/*
+	 * The task running blktrace is supposed to shutdown blktrace
+	 * by ioctl. If they forget to shutdown or can't do it because
+	 * of OOM or sort of situation, we shutdown for them.
+	 */
+	if (blkdev_has_run_blktrace(filp))
+		blk_trace_shutdown(bdev->bd_disk->queue);
+
 	blkdev_put(bdev, filp->f_mode);
 	return 0;
 }
@@ -1664,6 +1674,8 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 	else
 		mode &= ~FMODE_NDELAY;
 
+	blkdev_mark_blktrace(file, cmd);
+
 	return blkdev_ioctl(bdev, mode, cmd, arg);
 }
 
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index a083e15df608..754058c1965c 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -135,4 +135,15 @@ static inline unsigned int blk_rq_trace_nr_sectors(struct request *rq)
 	return blk_rq_is_passthrough(rq) ? 0 : blk_rq_sectors(rq);
 }
 
+static inline void blkdev_mark_blktrace(struct file *file, unsigned int cmd)
+{
+	if (cmd == BLKTRACESETUP)
+		file->private_data = (void *)-1;
+}
+
+static inline bool blkdev_has_run_blktrace(struct file *file)
+{
+	return file->private_data == (void *)-1;
+}
+
 #endif
-- 
2.29.2


-- 
Ming