staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse
PATCH 1/1 staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse
The third argument to function kportal_memhog_alloc is expected to be gfp_t whereas the actual argument was unsigned int. Fix this by explicitly typecasting to gfp_t
--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/lustre/lustre/libcfs/module.c +1 -1
@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */ err =kportal_memhog_alloc (pfile->private_data, data->ioc_count, - data->ioc_flags); + (__force gfp_t )data->ioc_flags);if (err !=0 )kportal_memhog_free (pfile->private_data); }
Signature · 1 line
1.9.1
Greg Kroah-Hartman
maintainerQuoted 14 lines · Niranjan Dighe
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0
drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 5 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */ err =kportal_memhog_alloc (pfile->private_data, data->ioc_count, - data->ioc_flags); + (__force gfp_t )data->ioc_flags);
No, please fix the type to be correct properly, like the comment says needs to be done.
thanks,
greg k-h
Quoted 24 lines · Greg Kroah-Hartman
Quoted 14 lines · Niranjan Dighe
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 5 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */ err =kportal_memhog_alloc (pfile->private_data, data->ioc_count, - data->ioc_flags); + (__force gfp_t )data->ioc_flags);No, please fix the type to be correct properly, like the comment says needs to be done.
Quoted 3 lines
thanks,
greg k-h
Hello Greg,
I could see that the ioc_flags member of the struct libcfs_ioctl_data is used as gfp_t only in the
case of the ioctl IOC_LIBCFS_MEMHOG. I can think of following ways to correct it -
1. Create a union that has 2 different types encapsulated, something like this -
union {
__u32 ioc_flags;
gfp_t alloc_flags;
}flags;Because, the ioc_flags seems to be used in different contexts at different places throughout thedrivers/staging/lustre directory.
2. Is it OK to hardcode the appropriate gfp_t flags for the IOC_LIBCFS_MEMHOG, as the userspace
seems to be taking the decision about the page allocation zone/strategy, is this what is intended?
Regards,
Niranjan Dighe
Greg Kroah-Hartman
maintainerQuoted 37 lines · Niranjan Dighe
Quoted 24 lines · Greg Kroah-Hartman
Quoted 14 lines · Niranjan Dighe
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 5 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */ err =kportal_memhog_alloc (pfile->private_data, data->ioc_count, - data->ioc_flags); + (__force gfp_t )data->ioc_flags);No, please fix the type to be correct properly, like the comment says needs to be done.
Quoted 3 lines
thanks,
greg k-h
Hello Greg,
I could see that the
ioc_flagsmember of the structlibcfs_ioctl_datais used asgfp_tonly in the
case of the ioctlIOC_LIBCFS_MEMHOG. I can think of following ways to correct it -
Quoted 8 lines
1. Create a union that has 2 different types encapsulated, something like this -
union { __u32 ioc_flags; gfp_t alloc_flags; }flags;Because, the
ioc_flagsseems to be used in different contexts at different places throughout thedrivers/staging/lustredirectory.
This crosses the user/kernel boundry? Hah, no, just drop it, don't touch this at all, lustre ioctls are a complete mess and need to be fixed up by the authors who can test them, just leave this alone for now, sorry.
greg k-h
Quoted 46 lines · Niranjan Dighe
Quoted 27 lines · Greg Kroah-Hartman
Quoted 20 lines · Niranjan Dighe
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */
err = kportal_memhog_alloc(pfile->private_data, data->ioc_count,drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 2 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1- data->ioc_flags); + (__force gfp_t)data->ioc_flags);No, please fix the type to be correct properly, like the comment says needs to be done.
Quoted 3 lines
thanks,
greg k-h
Hello Greg,
I could see that the
ioc_flagsmember of the structlibcfs_ioctl_datais used asgfp_tonly in the
case of the ioctlIOC_LIBCFS_MEMHOG. I can think of following ways to correct it -1. Create a union that has 2 different types encapsulated, something like this -
union { __u32 ioc_flags; gfp_t alloc_flags; }flags;
Quoted 8 lines
Because, the
ioc_flagsseems to be used in different contexts at different places throughout thedrivers/staging/lustredirectory.2. Is it OK to hardcode the appropriate
gfp_tflags for theIOC_LIBCFS_MEMHOG, as the userspace
seems to be taking the decision about the page allocation zone/strategy, is this what is intended?
The memhog functionality is used to introduce memory pressure on a client or server during operation to test error handling as well as memory allocation deadlocks (e.g. GFP_KERNEL used where GFP_NOFS should be used). There are other ways to do this in the kernel today, so all of the memhog code could just be deleted I think.
This looks like kportal_memhog_alloc(), kportal_memhog_free(), IOC_LIBCFS_MEMHOG, and struct libcfs_device_userstate could be removed.
Cheers, Andreas
Quoted 63 lines · Andreas
Quoted 46 lines · Niranjan Dighe
Quoted 27 lines · Greg Kroah-Hartman
Quoted 20 lines · Niranjan Dighe
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */
err = kportal_memhog_alloc(pfile->private_data, data->ioc_count,drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 2 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1- data->ioc_flags); + (__force gfp_t)data->ioc_flags);No, please fix the type to be correct properly, like the comment says needs to be done.
Quoted 3 lines
thanks,
greg k-h
Hello Greg,
I could see that the
ioc_flagsmember of the structlibcfs_ioctl_datais used asgfp_tonly in the
case of the ioctlIOC_LIBCFS_MEMHOG. I can think of following ways to correct it -1. Create a union that has 2 different types encapsulated, something like this -
union { __u32 ioc_flags; gfp_t alloc_flags; }flags;Quoted 8 lines
Because, the
ioc_flagsseems to be used in different contexts at different places throughout thedrivers/staging/lustredirectory.2. Is it OK to hardcode the appropriate
gfp_tflags for theIOC_LIBCFS_MEMHOG, as the userspace
seems to be taking the decision about the page allocation zone/strategy, is this what is intended?The memhog functionality is used to introduce memory pressure on a client or server during operation to test error handling as well as memory allocation deadlocks (e.g.
GFP_KERNELused whereGFP_NOFSshould be used). There are other ways to do this in the kernel today, so all of the memhog code could just be deleted I think.
Quoted 6 lines
This looks like
kportal_memhog_alloc(),kportal_memhog_free(),IOC_LIBCFS_MEMHOG, and structlibcfs_device_userstatecould be removed.Cheers, Andreas
Thanks Andreas, I will send out a separate patch with the cleanup as you suggested.
Regards,
Niranjan
Quoted 26 lines
Quoted 26 lines · Niranjan Dighe
Quoted 14 lines
The third argument to function
kportal_memhog_allocis expected to begfp_twhereas the actual argument was unsigned int. Fix this by explicitly typecasting togfp_tSigned-off-by Niranjan Dighe <[email protected]>--- drivers/staging/lustre/lustre/libcfs/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lustre/libcfs/module.c +0
-0drivers/staging/lustre/lustre/libcfs/module.c static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd, · line 271
Quoted 5 lines
drivers/staging/lustre/lustre/libcfs/module.c +1
-1@@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file *pfile, unsigned long cmd,/* XXX The ioc_flags is not GFP flags now, need to be fixed */ err =kportal_memhog_alloc (pfile->private_data, data->ioc_count, - data->ioc_flags); + (__force gfp_t )data->ioc_flags);No, please fix the type to be correct properly, like the comment says needs to be done.
thanks,
Quoted 8 lines
Quoted 1 line
greg k-h
Hello Greg,
I could see that the
ioc_flagsmember of the structlibcfs_ioctl_datais used asgfp_tonly in the
case of the ioctlIOC_LIBCFS_MEMHOG. I can think of following ways to correct it -
IOC_LIBCFS_MEMHOG will be going away. Since this keeps coming up I will prepare some patches. Especially now that out tools no longer uses these obsolete ioctls.
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
2. Is it OK to hardcode the appropriate
gfp_tflags for theIOC_LIBCFS_MEMHOG, as the userspace
seems to be taking the decision about the page allocation zone/strategy, is this what is intended?The memhog functionality is used to introduce memory pressure on a client or server during operation to test error handling as well as memory allocation deadlocks (e.g.
GFP_KERNELused whereGFP_NOFSshould be used). There are other ways to do this in the kernel today, so all of the memhog code could just be deleted I think.
Quoted 8 lines
Quoted 6 lines
This looks like
kportal_memhog_alloc(),kportal_memhog_free(),IOC_LIBCFS_MEMHOG, and structlibcfs_device_userstatecould be removed.Cheers, Andreas
Thanks Andreas, I will send out a separate patch with the cleanup as you suggested.
I missed this email. I just sent the cleanup patches a bit ago.