FreeLists / openbeosstorage / [openbeosstorage] Re: DiskDevice API 2.x, Kernelland Draft

["Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>]

First of all, I haven't had a deep look at it yet, because I didn't 
find the time. But I somehow wanted to answer this one now :)

Tyler Dauwalder <tyler@xxxxxxxxxxxxx> wrote:
> > When BDiskDevice::CommitModifications() (the bracketing method for
> > PrepareModifications()) is invoked, the shadow hierarchy is 
> > compared
> > with the original one and respective jobs (KDiskDeviceJob) are 
> > created
> > and scheduled (put into a KDiskDeviceJobQueue). 
> This seems a bit odd to me. Shouldn't the shadow hierachy just keep 
> track of the appropriate job queue necessary to reach the state of 
> the 
> shadow hierarchy from the state of the original hierachy? I don't see 
> how one could easily infer a proper scheduling job sequence from just 
> the beginning and ending states of the hierarchy.

As long as we can't change the nested structure, it would be pretty 
simple, because the partitions are easily identified by their ID - and 
there are only two methods, moving and resizing, which can easily be 
differentiated.
Having those shadow partitions (although I don't like the name much, 
something like "target partitions" would make clearer that the current 
partitions should be changed) would greatly simplify the handling of 
the job queue from userland.
That way, as a user, you can fiddle around with the partitions to no 
end; there is no reason you change them directly from one state to 
another. You first shrink one partition, then move another one around, 
then shrink the first one a bit more, etc.

[...]
> > initialize_partition() does perhaps need a bit more discussion, 
> > since
> > there exists the planned fs_initialize_volume() function (<be/
> > kernel/
> > fs_volume.h>), which has largely intersecting functionality (cf.
> > userland_interface.h for some more thoughts).
> My vote is for ditching fs_initialize_volume() and adding support for 
> registering files as disk devices. What would the arguments be for 
> keeping fs_initialize_volume() (other than the regular file problem)?

No, actually, that would be kinda stupid IMO. A file system needs a 
device (or file) to initialize its structure on. It starts at 0 and has 
the length of the whole device (partition or file). Anything else would 
make it complicated.
Now, why should we hide the direct method of initializing a file 
system, and force the user to get the whole BPartition tree, the need 
to search for the right partition, disabling the possibility of 
creating file systems in regular files, etc.
I would rather remove the initialize_partition() function, and have 
something like (I would guess it already exists):
status_t BDiskDeviceList::GetPartition(BPartition &partition, const 
char *deviceName);
(dunno if this class would be the right container, though)

status_t BPartition::GetDeviceName(char *deviceName);

and then just call fs_initialize_volume() using that deviceName.

> > As written in KDiskDeviceJob.h, I'm not sure if we want to add a
> > Cancel() method to cancel a job in progress. Scheduled jobs can 
> > easily
> > be canceled by removing them from the job queue. I just realize, 
> > that
> > we don't provide any methods in the userland API for canceling a 
> > job.
> Yes, I think we should support cancel, for things like defragging 
> that 
> can be interrupted. But we should also allow the job to specify if it 
> allows cancelling or not. 

We can cancel all jobs - there would pop up a requester which says 
"Canceling the operation will recreate the initial state - this can 
take a while", and just reverse the thing.
Shouldn't be too hard, at least not for moving a partition around.
How ugly it gets for resizing a partition would be the job of the file 
system to judge on. But since we already need to have logging for those 
jobs, reversing the operation at any point shouldn't be impossible (or 
too ugly) at all.

> > 3.3 FS Add-Ons
> > 
> > The FS interface functions are similar to the ones of partition 
> > module
> > interface, except for a few differences: To fit better into the 
> > rest 
> > of the FS add-on interface, none of functions is passed a file 
> > descriptor, but a path of the partition device instead. 
> Wouldn't it be better to just open the proper device once and pass it 
> around? This way you could also ensure the device is only accessed 
> read-only during scanning. 

It could be nice to have this, and it would also be a bit faster. I am 
not sure if it's rendering the API inconsistent there, though. One 
could argue that the partition related functions (not initialize()) are 
a bit separated from the rest anyway.

Adios...
   Axel.