[RFC,01/18] blktrace: increase the size of action mask
diff mbox series

Message ID 20190501042831.5313-2-chaitanya.kulkarni@wdc.com
State New
Headers show
Series
  • blktrace: add blktrace extension support
Related show

Commit Message

Chaitanya Kulkarni May 1, 2019, 4:28 a.m. UTC
This patch adds the blktrace extension support where we increase the
size of action mask so that it can store more actions.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/linux/blktrace_api.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Bart Van Assche May 1, 2019, 3:48 p.m. UTC | #1
On Tue, 2019-04-30 at 21:28 -0700, Chaitanya Kulkarni wrote:
> -#define BLKTRACESETUP32 _IOWR(0x12, 115, struct compat_blk_user_trace_setup)
> +
> +/* XXX: temp work around for RFC */
> +#define BLKTRACESETUP32 _IOWR(0x13, 115, struct compat_blk_user_trace_setup)

This change breaks user space so this change is not acceptable. I think you
want to introduce a new ioctl instead of modifying an existing ioctl.
Additionally, have you considered to split the blktrace_api.h header file
into two header files: one with kernel-internal definitions and a second one
with definitions that are shared with user space (include/uapi/...)?

Thanks,

Bart.
Chaitanya Kulkarni May 2, 2019, 3:43 a.m. UTC | #2
On 5/1/19 8:48 AM, Bart Van Assche wrote:
> On Tue, 2019-04-30 at 21:28 -0700, Chaitanya Kulkarni wrote:
>> -#define BLKTRACESETUP32 _IOWR(0x12, 115, struct compat_blk_user_trace_setup)
>> +
>> +/* XXX: temp work around for RFC */
>> +#define BLKTRACESETUP32 _IOWR(0x13, 115, struct compat_blk_user_trace_setup)
> 
> This change breaks user space so this change is not acceptable. I think you
> want to introduce a new ioctl instead of modifying an existing ioctl.
> Additionally, have you considered to split the blktrace_api.h header file
> into two header files: one with kernel-internal definitions and a second one
> with definitions that are shared with user space (include/uapi/...)?
> 
> Thanks,
> 
> Bart.
> 

I want to avoid modifying an existing IOCTL, I'll add a new ioctl and 
update the tools to use the extension IOCTL and split the header file 
also. Also I found that user space tools have replicated BLK_XX_XXX 
definitions, will be okay to keep all those in one place and include 
those from the appropriate header file ?
Bart Van Assche May 2, 2019, 3:12 p.m. UTC | #3
On Thu, 2019-05-02 at 03:43 +0000, Chaitanya Kulkarni wrote:
> On 5/1/19 8:48 AM, Bart Van Assche wrote:
> > On Tue, 2019-04-30 at 21:28 -0700, Chaitanya Kulkarni wrote:
> > > -#define BLKTRACESETUP32 _IOWR(0x12, 115, struct compat_blk_user_trace_setup)
> > > +
> > > +/* XXX: temp work around for RFC */
> > > +#define BLKTRACESETUP32 _IOWR(0x13, 115, struct compat_blk_user_trace_setup)
> > 
> > This change breaks user space so this change is not acceptable. I think you
> > want to introduce a new ioctl instead of modifying an existing ioctl.
> > Additionally, have you considered to split the blktrace_api.h header file
> > into two header files: one with kernel-internal definitions and a second one
> > with definitions that are shared with user space (include/uapi/...)?
> 
> I want to avoid modifying an existing IOCTL, I'll add a new ioctl and 
> update the tools to use the extension IOCTL and split the header file 
> also. Also I found that user space tools have replicated BLK_XX_XXX 
> definitions, will be okay to keep all those in one place and include 
> those from the appropriate header file ?

Hi Chaitanya,

I think all definitions that are relevant for the user space blktrace tool
should be moved into a header file under include/uapi/linux. I'm not sure
what the best strategy is to include that header file in the blktrace tool.
Another project that interfaces with the kernel (rdma-core; see also
https://github.com/linux-rdma/rdma-core/) periodically copies kernel header
files into its own source code repository.

Bart.

Patch
diff mbox series

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 7bb2d8de9f30..403d4cfc6a52 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -17,7 +17,11 @@  struct blk_trace {
 	struct rchan *rchan;
 	unsigned long __percpu *sequence;
 	unsigned char __percpu *msg_data;
+#ifdef CONFIG_BLKTRACE_EXT
+	u64 act_mask;
+#else
 	u16 act_mask;
+#endif /* CONFIG_BLKTRACE_EXT */
 	u64 start_lba;
 	u64 end_lba;
 	u32 pid;
@@ -101,14 +105,20 @@  static inline int blk_trace_init_sysfs(struct device *dev)
 
 struct compat_blk_user_trace_setup {
 	char name[BLKTRACE_BDEV_SIZE];
+#ifdef CONFIG_BLKTRACE_EXT
+	u64 act_mask;
+#else
 	u16 act_mask;
+#endif /* CONFIG_BLKTRACE_EXT */
 	u32 buf_size;
 	u32 buf_nr;
 	compat_u64 start_lba;
 	compat_u64 end_lba;
 	u32 pid;
 };
-#define BLKTRACESETUP32 _IOWR(0x12, 115, struct compat_blk_user_trace_setup)
+
+/* XXX: temp work around for RFC */
+#define BLKTRACESETUP32 _IOWR(0x13, 115, struct compat_blk_user_trace_setup)
 
 #endif