Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

On Mon, Sep 06, 2021 at 02:11:31PM -0600, Jens Axboe wrote:

On 8/29/21 8:37 AM, Lukas Prediger wrote:

quoted

The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
global state, meaning that only one process can detect a disc change
while the ioctl call will return 0 for other calling processes afterwards
(see bug 213267 ).

This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
works by maintaining a timestamp of the last detected disc change instead
of a boolean flag: Processes calling this ioctl command can provide
a timestamp of the last disc change known to them and receive
an indication whether the disc was changed since then and the updated
timestamp.

I considered fixing the buggy behavior in the original
CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
for each calling process in the kernel, which seems like a worse
solution than introducing this new ioctl.

This looks pretty good to me now. Adding Phillip to the CC, he's the new
CDROM maintainer. Leaving the rest of the message below intact because
of that.

quoted

...

quoted

Dear Lukas,

Thank you for the patch, much appreciated and looks great. One very
minor thing though has jumped out at me after running checkpatch though:

quoted

--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h

@@ -147,6 +147,8 @@
 #define CDROM_NEXT_WRITABLE  0x5394  /* get next writable block */
 #define CDROM_LAST_WRITTEN   0x5395  /* get last block written on disc */

+#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
+
 /*******************************************************
  * CDROM IOCTL structures
  *******************************************************/

@@ -295,6 +297,19 @@ struct cdrom_generic_command
      };
 };

+/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
+struct cdrom_timed_media_change_info
+{

This opening brace should be on the same line as the struct definition
as per current style rules.

I also got a checkpatch warning about ENOSYS being used as an error
value, but I can see this usage is standard in the driver and not a
problem so no issue with that.

I will review and test properly after work tomorrow (being new to the
role I'd like to make sure I'm taking the proper time), but I have no
doubt it will work fine. Assuming it does I will be happy to accept the
patch with the above brace change. Thanks again.

quoted

+	__s64	last_media_change;	/* Timestamp of the last detected media
+					 * change in ms. May be set by caller, updated
+					 * upon successful return of ioctl.
+					 */
+	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
+					 * change was more recent than
+					 * last_media_change set by caller.
+					 */
+};
+
 /*
  * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336, 
  * 2340, or 2352 bytes long.  

Regards,
Phil