Page MenuHomeFreeBSD

Make BIO_SPEEDUP opt-in
AbandonedPublic

Authored by imp on Feb 6 2020, 8:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 30 2023, 8:43 PM
Unknown Object (File)
Dec 23 2023, 1:10 AM
Unknown Object (File)
Oct 13 2023, 6:00 PM
Unknown Object (File)
Oct 12 2023, 10:18 PM
Unknown Object (File)
Aug 16 2023, 7:28 AM
Unknown Object (File)
Jun 13 2023, 5:02 AM
Unknown Object (File)
May 30 2023, 10:49 AM
Unknown Object (File)
May 27 2023, 2:26 PM
Subscribers

Details

Summary

There's a lot of disk drivers in the tree, many don't like random
commands. So, like BIO_DELETE, make it opt-in only. The drivers that
are part of the CAM IOSCHEDULER are the only ones that can use
this. For everybody else, return success, but don't adjust the
residual to indicate that nothing was done in response.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29213
Build 27139: arc lint + arc unit

Event Timeline

I would honestly prefer drivers to report reasonable errors on unknown commands. That would fix the issue once and for all.

In D23541#516762, @mav wrote:

I would honestly prefer drivers to report reasonable errors on unknown commands. That would fix the issue once and for all.

Some do, some panic, some get confused... I didn't feel up to fixing all that today.

I won't object too much, but first addition of quite specialized command and then workaround for it does not make me particularly happy.

In D23541#516766, @mav wrote:

I won't object too much, but first addition of quite specialized command and then workaround for it does not make me particularly happy.

I understand. Had I realized the issue before the commit, I'd have done this from the start. I have an AI to make it so we can add additional commands in the future w/o panics.

scottl requested changes to this revision.Feb 7 2020, 5:33 AM

I agree with Alexander, I really don't like that this perpetuates bad design.

This revision now requires changes to proceed.Feb 7 2020, 5:33 AM

I agree with Alexander, I really don't like that this perpetuates bad design.

So the proper fix is to fix all the disk drivers to just return op not supported?

In D23541#516964, @imp wrote:

I agree with Alexander, I really don't like that this perpetuates bad design.

So the proper fix is to fix all the disk drivers to just return op not supported?

Yep, gotta bite the bullet sometime

--- scsi_da.c	(revision 355435)
+++ scsi_da.c	(working copy)
@@ -3381,6 +3381,10 @@
 			}
 			break;
 		}
+		default:
+			biofinish(bp, NULL, EOPNOTSUPP);
+			xpt_release_ccb(start_ccb);
+			return;
 		}
 		start_ccb->ccb_h.ccb_state = DA_CCB_BUFFER_IO;
 		start_ccb->ccb_h.flags |= CAM_UNLOCKED;
In D23541#516964, @imp wrote:

I agree with Alexander, I really don't like that this perpetuates bad design.

So the proper fix is to fix all the disk drivers to just return op not supported?

Yep, gotta bite the bullet sometime

--- scsi_da.c	(revision 355435)
+++ scsi_da.c	(working copy)
@@ -3381,6 +3381,10 @@
 			}
 			break;
 		}
+		default:
+			biofinish(bp, NULL, EOPNOTSUPP);
+			xpt_release_ccb(start_ccb);
+			return;
 		}
 		start_ccb->ccb_h.ccb_state = DA_CCB_BUFFER_IO;
 		start_ccb->ccb_h.flags |= CAM_UNLOCKED;

Ok. I came up with the same code in ada :) I'll do a sweep tomorrow

In that case what about the console spam? I presume messages of this sort:

g_vfs_done():md10a[UNKNOWN()]error = 45

stem precisely from this. This happens to really spam the console for me when e.g., running stress2 when at least for this case it should never print anything. iow it looks like a design error that that devices which don't support something can get a request AND fail resulting in an error message.

Here is an example from pho's log: https://people.freebsd.org/~pho/stress/log/mark125.txt

In D23541#516975, @mjg wrote:

In that case what about the console spam? I presume messages of this sort:

g_vfs_done():md10a[UNKNOWN()]error = 45

stem precisely from this. This happens to really spam the console for me when e.g., running stress2 when at least for this case it should never print anything. iow it looks like a design error that that devices which don't support something can get a request AND fail resulting in an error message.

Here is an example from pho's log: https://people.freebsd.org/~pho/stress/log/mark125.txt

Good point, the block layer shouldn't be spamming like this.

In D23541#516975, @mjg wrote:

In that case what about the console spam? I presume messages of this sort:

g_vfs_done():md10a[UNKNOWN()]error = 45

stem precisely from this. This happens to really spam the console for me when e.g., running stress2 when at least for this case it should never print anything. iow it looks like a design error that that devices which don't support something can get a request AND fail resulting in an error message.

Here is an example from pho's log: https://people.freebsd.org/~pho/stress/log/mark125.txt

Good point, the block layer shouldn't be spamming like this.

I'll fix that...

We'll fix a different way