Mellvik / TLVC

Tiny Linux for Vintage Computers
Other
7 stars 0 forks source link

[raw IO] Cleanup in the raw HD driver. #30

Closed Mellvik closed 9 months ago

Mellvik commented 9 months ago

Change the buffer_head struct to always include the elements formerly only included when using external buffers, notably b_L2seg which is required by the raw hd driver.

Also takes advantage of the 'available' byte in the struct to hold the number of sectors (b_nr_sectors) for raw IO operations, formerly encoded into the b_count field.

The next step in this is to remove the CONFIG_BLK_DEV_CHAR selection entirely, making raw access an implicit part of the HDdriver (end soon, the FD driver). In turn a preparation for changes to the system disk utilities (mkfs, fdisk, ...) to always use raw access.

ghaerr commented 9 months ago

I can see the somewhat extreme gyrations you've been having to go through to communicate from the raw disk driver to the block device driver. This PR is certainly much better with the prior changes removed, however IMO - there is still a better way.

A while back, when you first introduced the raw device driver (something that ELKS doesn't have), I realized that what is really needed to make that work (conceptually) is an LBA sector number, number of sectors, and a far buffer address. At that time, I rewrote all ELKS block device drivers to use rq_sector, rq_nr_sectors, rq_seg and rq_buffer. The block number is now converted to a start sector number and number of sectors before the block driver sees it. All this was a pretty small modification. Around the same time you found more buffer subsystem problems, I realized that we didn't need multiple segment/buffer addresses in the buffer header, and those were removed.

Now - in your case, for the raw disk driver, I see an easy path forward that doesn't require adding buffer header variables back (with have nothing to do with buffers) etc: Don't use ll_rw_blk to do the I/O, and don't use a buffer header to communicate with the block driver. Instead, write a BRAND NEW routine that looks very similar to make_request that takes the raw device start sector, number of sectors, and buffer address, and copies them directly into the I/O request structure, set rq_bh to NULL, queue the request and sleep. Then, instead of calling wait_on_buffer, special case end_request to wakeup on the new wait queue, and only do this when rq_bh == NULL.

Very clean, and no need for any additional variables. rq_sector, rq_nr_sectors always mean exactly the same thing, in both the raw and block driver requests. All block device drivers automatically become able to handle raw requests without any modifications.

My $0.02! :)

Mellvik commented 9 months ago

Now - in your case, for the raw disk driver, I see an easy path forward that doesn't require adding buffer header variables back (with have nothing to do with buffers) etc: Don't use ll_rw_blk to do the I/O, and don't use a buffer header to communicate with the block driver. Instead, write a BRAND NEW routine that looks very similar to make_request that takes the raw device start sector, number of sectors, and buffer address, and copies them directly into the I/O request structure, set rq_bh to NULL, queue the request and sleep. Then, instead of calling wait_on_buffer, special case end_request to wakeup on the new wait queue, and only do this when rq_bh == NULL.

Very clean, and no need for any additional variables. rq_sector, rq_nr_sectors always mean exactly the same thing, in both the raw and block driver requests. All block device drivers automatically become able to handle raw requests without any modifications.

My $0.02! :)

Thanks @ghaerr - appreciated. Your thinking goes quite a bit further than the experiments I did while trying to extend the reach of req-> 's back to the ll_ routines in the early days. It became too complicated. Your approach is much simpler (and by the way, I've learned a lot since those early March days this year) - and that may actually be the way to go.

However - there is a caveat. Take a look at raw_blk_rwin block_dev.cand you'll see what I'm referring to. A raw access must be able to read/write any number of bytes at any offset. So there may be partial sectors at both ends of the transfer. Not complicated, but it was convenient to use a regular buffer for it because the infrastructure is rigged already. From there to fake a buffer header for the main part of the transaction was just a win - plus the kludges you've seen recently. Of course, there is nothing in the scheme you suggest that makes this difficult - it's just a consideration when weighing in benefit/effort.

ghaerr commented 9 months ago

Take a look at raw_blk_rwin block_dev.c

Yes - now I see how you got pulled into using ll_rw_blk, given the need for offset bytes within a sector.

I've learned a lot since those early March days this year

Yes - me too! One of the things I've learned is to not have the same variables in two locations - which is why I removed b_seg and b_L2data. Although it seemed simple when I first wrote it, it ended up getting terribly complicated having to remember which exact cases were valid for each variable, etc. When I had to rewrite the buffer code for the problems you exposed, it just became too much, and (funnily enough), I was able to see that the extra variables weren't in fact needed and a very nice simplification was written, which enhanced reliability since I could barely follow my original code to guarantee correctness.

When you we first talked about your desire to add raw devices, I remember you saying: "raw devices should have nothing to do with system buffers". At first I didn't agree, but now more so than ever, I think your original thinking is on the right track - the raw device handling system doesn't need buffers and shouldn't pull them into it, even for convenience. The raw system could completely bypass all that and talk directly with the device driver. Kind of neat, unless it gets too complicated.

Mellvik commented 9 months ago

When you we first talked about your desire to add raw devices, I remember you saying: "raw devices should have nothing to do with system buffers". At first I didn't agree, but now more so than ever, I think your original thinking is on the right track - the raw device handling system doesn't need buffers and shouldn't pull them into it, even for convenience. The raw system could completely bypass all that and talk directly with the device driver. Kind of neat, unless it gets too complicated.

That's definitely still the goal. That comes after adding raw to the floppy driver though. Actually - the quick'n'dirty way to do that is to fake it: Create a local buffer header, alloc a block for the odd ends (in block_dev.c) and a few adjustments further down.

Thanks.