Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

14.12.2021 14:45, Michal Suchánek wrote:
> On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
>> floppy drive works bad when opened with O_NONBLOCK: first read may
>> fail. This cause probing fail and leave error messages in dmesg. So, if
>> we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.
>>
>> Reproduce is simple:
>> 1. start the linux system (kernel should include the mentioned commit)
>>     in QEMU virtual machine with floppy device and with floppy disk
>>     inserted.
>> 2. If floppy module is not inserted, modprobe it.
>> 3. Try "blkid /dev/fd9": it will show nothing, errors will appear in
>>     dmesg
>> 4. Try "mount /dev/fd0 /mnt": it may succeed (as mount not only probing
>>     but also try filesystems one by one, if you have vfat in
>>     /etc/filesytems or in /proc/filesystems, mount will succeed), but
>>     errors about failed read still appear in dmesg, as probing was done.
>>
>> Mentioned errors in dmesg looks like this:
>>   floppy0: disk absent or changed during operation
>>   blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
>>   floppy0: disk absent or changed during operation
>>   blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>>   Buffer I/O error on dev fd0, logical block 0, async page read
>>
>> Note also, that these errors also appear in early dmesg messages, if
>> probing is done on system startup. For example, it happens when
>> cloud-init package is installed.
>>
>> Note2: O_NONBLOCK flag for probing is used since
>> 39f5af25982d8b0244000e92a9d0e0e6557d0e17
>> "libblkid: open device in nonblock mode", which was done to fix the
>> issue with cdrom: if tray is open and we call open() without O_NONBLOCK
>> the tray may be automatically closed, which is not what we want in
>> blkid.
>>
>> Good discussion on this bug is here:
>> https://bugzilla.suse.com/show_bug.cgi?id=1181018
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Note, that this commit is done as a "minimal change", i.e. I only try to
>> rollback O_NONBLOCK for floppy. The other way is to detect CDROM
>> instead, and reopen with original flags for everything except CDROM.
>>
>> I also tried fcntl instead of close/open, and that didn't help.
>>
>>   libblkid/src/probe.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
>> index 70e3dc0eb..68a644597 100644
>> --- a/libblkid/src/probe.c
>> +++ b/libblkid/src/probe.c
>> @@ -94,6 +94,9 @@
>>   #ifdef HAVE_LINUX_CDROM_H
>>   #include <linux/cdrom.h>
>>   #endif
>> +#ifdef HAVE_LINUX_FD_H
>> +#include <linux/fd.h>
>> +#endif
>>   #ifdef HAVE_LINUX_BLKZONED_H
>>   #include <linux/blkzoned.h>
>>   #endif
>> @@ -200,10 +203,32 @@ blkid_probe blkid_clone_probe(blkid_probe parent)
>>    * We add O_NONBLOCK flag to the mode, as opening CDROM without this flag may
>>    * load to closing the rom (if it's open), which is bad thing in context of
>>    * blkid: we don't want to change the actual device state.
>> + *
>> + * Still, since c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> + * "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel, floppy
>> + * drive works bad when opened with O_NONBLOCK: first read may fail. This cause
>> + * probing fail and leave error messages in dmesg. So, if we detect that opened
>> + * fd is floppy, reopen it without O_NONBLOCK flag.
>>    */
>>   int blkid_safe_open(const char *filename, int mode)
>>   {
>> -	return open(filename, mode | O_NONBLOCK);
>> +	int fd = open(filename, mode | O_NONBLOCK);
>> +	if (fd < 0) {
>> +		return fd;
>> +	}
>> +
>> +#ifdef FDGETDRVTYP
>> +	{
>> +		char name[1000];
> Hello,
> 
> I wonder if it's better to use FDGETFDCSTAT which seems to be meant as
> stable API.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fd.h#n271
> 
> As is this allocates 1k from stack and can be presumably called from
> application context with arbitrarily deep stack so it seems a bit
> wasteful. floppy_fdc_state has under 60 bytes.
> 
> Also if you are not interested in the result you can make the buffer
> static. Not sure it makes sense to bother if the buffer size is
> reasonable.
> 

Thanks for advice, will do for v2. I'm rather far of kernel development, and not fan of floppy disks:) So my choice was "something I can understand from drivers/block/floppy.c and which never fail". I tried FDGETPRM (because it used somewhere else in util-linux), but it fails in described case.. and in kernel code it's obvious that it may fail.

> 
>> +
>> +		if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {
>> +			close(fd);
>> +			fd = open(filename, mode);
>> +		}
>> +	}
>> +#endif /* FDGETDRVTYP */
>> +
>> +	return fd;
>>   }
>>   
>>   
>> -- 
>> 2.31.1
>>


-- 
Best regards,
Vladimir