Re: DiskDevice API 2.x, Kernelland Draft

> [...]
> > > > > > 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.
> > > 
> > > Don't know, if that's necessary. Jobs, that can't be cancelled,
> > > will
> > > simply finish their task.
> > 
> > So are you proposing we just always display cancellation as an
> > option,
> > and if the task doesn't support it, things just keep quietly 
> > plugging
> > away when you click the "Cancel" button? I think we should offer the
> > user some indication of which tasks can be canceled gracefully and
> > which can't.
> 
> OK, why not. So, how about adding a uint32
> BDiskDeviceJob::CancelProperties(), which would return a combination 
> of
> these:
> 
> enum {
>     B_DISK_DEVICE_JOB_CAN_CANCEL            = 0x01,
>     B_DISK_DEVICE_JOB_STOP_ON_CANCEL        = 0x02,
>     B_DISK_DEVICE_JOB_REVERSE_ON_CANCEL        = 0x04,
> };
> 
> The cancel method would have parameters `bool force' and `bool
> reverse'. It wouldn't have any effect, when
> B_DISK_DEVICE_JOB_CAN_CANCEL is not set. If `force' is true the
> `reverse' parameter is ignored and the job is told to quit as soon as
> possible, even, if that means, that the partition will be left in an
> invalid state. Otherwise `reverse' indicates whether the effect of the
> job shall be undone.
> 
> CancelProperties() would be live. It could change during the execution
> of the job (e.g. repairing might have a read-only analysis phase that
> can simply be canceled, while canceling the repairing phase might 
> leave
> the partition in an invalid state).

Okay, works for me.

> > commit_disk_device_modifications(device, ...);
> >     // this will finally trigger the modifications to be made
> >     // and unlock the API - or will it first process all jobs
> >     // and the unlock the API?
> 
> It will compare the original with the shadow hierarchy and create and
> schedule the jobs needed to transform the original hierarchy
> respectively. The shadow hierarchy is deleted, and all partitions that
> might be affected by any scheduled job are marked `busy', all 
> ancestors
> of `busy' partitions are marked `descendant-busy'. When the syscall
> returns, a subsequent prepare_disk_device_modifications() on that disk
> device will succeed -- only partitions that are not marked `*busy' are
> allowed to be modified then, though.
> 
> > Where do we need the shadow partition name anyway? It doesn't seem 
> > to
> > be part of the user API.
> 
> You mean KPartition::Name()? If one sets the name of a partition via
> the userland API, it should also be reflected in the partition data, I
> think. We currently don't have the functionality to set the name of a
> partition in the userland API. We may want to add it, though. Maybe we
> also want to discriminate between Name() and ContentName() (e.g.
> partition name vs. FS/volume name).

Yes, we probably should allow both, since they both have accessors 
already.

> > So the user will never
> > come across all those user_ prefixes - because if he would, I would
> > consider dumping them.
> 
> I introduced the prefix for partition_data and disk_device_data,
> because there're equally named structures for the partition module/FS
> add-on interfaces and KPartition/KDiskDevice will probably have to 
> deal
> with both. For consistency I added he prefix to the other userland
> structures as well.
> 
> > And if we have all these int32 IDs we could think about adding a new
> > type for them, like partition_id.
> 
> Good idea. What about IDs for disk systems and disk device jobs?
> disk_system_id, disk_device_job_id? Too long?

How about disk_job_id? Or just job_id, unless you're worried that's too 
general.

> > We might also add a "name" field to initialize_partition() since
> > almost
> > all disk systems share this property (IIRC only Intel style
> > partitioning doesn't have it).
> 
> If we do that, we should also make the functionality to set the name
> explicit (or even `name' and `content name').

Might be a good idea, since it's a pretty ubiquitous concept. And 
particularly since Tracker lets you do this by just renaming a volume.

> > As you mentioned, the only problem which we would still have is with
> > the image files. Though I would really like us to be able to simply
> > register/publish them as a device, it would also be nice to create a
> > file system on them without having to register them.
> > For example, that would be the use for the fs_shell, but perhaps 
> > also
> > other things, although I don't have a good idea right now.
> > Or to say it this way: I like to have no real differentiation 
> > between
> > block devices and files - they are almost the same from the OS point
> > of
> > view, why shouldn't we keep this? Registering a file as device would
> > be
> > certainly a step into the right direction - but it would be even
> > nicer
> > if that wouldn't be necessary at all.
> > We would also need to differentiate between file system images and
> > disk
> > images - but I guess we're doing this anyway.
> > 
> > Well, I think it would be okay to not consider the fs_shell case, 
> > but
> > to have mkfs register files automatically - the only thing we should
> > support, though, is that we should continue to be able to address
> > this
> > file using its standard path. I.e. something like:
> > $ mkfs test.image
> > $ makebootable test.image
> > 
> > Shouldn't fail even if "makebootable" needs a device to operate on.
> > Dunno how we should do that right now, though. Maybe it's not even a
> > top priority - but it'd be nice to have it.
> 
> OK, then how about the following additions to BDiskDeviceRoster?
> 
> partition_id RegisterDeviceFile(const char *filename);
>     // publishes: /dev/disk/virtual/files/<disk device ID>/raw
> status_t UnregisterDeviceFile(const char *filename);
> status_t UnregisterDeviceFile(partition_id device);
> 
> partition_id GetDeviceForPath(const char *filename, BDiskDevice *
> device,
>                               bool registerIfFile = false);
> partition_id GetPartitionForPath(const char *filename, BDiskDevice *
> device,
>                                  BPartition **partition,
>                                  bool registerIfFile = false);
> 
> mkfs (or fs_initialize_volume(), if we implement it using the
> DiskDevice API) would only have to call one method to get hold of the
> BPartition in questions. So, that should be rather convenient, I
> suppose.

Seems okay to me. :-)

> > > > > 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.
> > > > Either way would work for me. So, if you have any preferences...
> > > I prefer passing the device. :-)
> > 
> > Okay, so let's do that, then :-)
> 
> So, then it's a FD plus the ID of the partition (something is needed 
> to
> identify it), I suppose.

Yes to the former, and yes to the latter where it's needed (the 
scanning functions could get away with the the FD, as the partition 
scanning functions do, couldn't they?).

-Tyler