[openbeosstorage] Re: DiskDevice API v2.1

"Axel D=F6rfler"  <axeld@xxxxxxxxxxxxxxxx> wrote:
> 
> Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx> wrote:
[...]
> > > The ValidateXYZ() functions would be used to bounds/error check
> > > modifications before actually solidifying said modification with 
> > > the
> > > corresponding XYZ() function. The BDiskDevice::CommitUpdates()
> > > function would then actually perform the device's batched 
> > > physical
> > > updates.
> > Maybe it would be better to have CommitUpdates() in BPartition 
> > rather 
> > than
> > in BDiskDevice. If we provided a partition locking, the siblings of 
> > the
> > affected partition wouldn't need to be locked. Not sure though.
> 
> The siblings might have to be updated as well - i.e. if you move a 
> partition around, its predecessor might need to update its "next" 
> link.

Moving and resizing are operations on the parent partition, anyway. So 
the parent partition has to be locked (too).

> I am not sure if it's worth the effort to provide partition-granular 
> locking - perhaps it's better to be able to add jobs to the current 
> update task=3D3F AFAICT all partition changes have to be sequential 
> anyway.

Changes to a single partition. Yes. But I actually don't see the reason 
for a sequence of changes to one partition. If requested by one 
application, it can as well be bundled to one action. And if the 
requests is issued by another application, it may operate with obsolete 
data anyway (imagine one app resizing the partition to occupy all free 
space, while the second one wants to move it) and should be forbidden. 
That's why I'd prefer a locking mechanism -- be it for individual 
partitions or for the whole device.

> We just should limit the number of jobs a single partition has 
> outstanding (i.e. only one resize change, one movie, etc.).

If requested by a single application, this application should bundle 
the changes anyway and commit them together. There would never be more 
than one active or pending job per device or partition.

> > > The BDiskSystem class provides info about supported child and 
> > > parent
> > > systems for each disk system (i.e. intel extended would require 
> > > an
> > > intel parent).
> > When introducing such a class, I would make it more complete. 
> > Actually,
> > those objects could be returned by the add-ons. The class should 
> > then 
> > have
> > ValidateXYZ(BPartition*,...) (implemented by derived classes), 
> > which 
> > are
> > invoked by their BPartition::ValidateXYZ() counterparts. This 
> > means, 
> > that
> > the add-ons for the used systems must be kept loaded while the API 
> > is
> > used, but that shouldn't harm that much, I think.
> > 
> > An alternative -- I guess, you envisioned it like that -- is to not 
> > make
> > the BDiskSystem methods virtual, but let them call into the kernel, 
> > where
> > the repective module is asked. I don't think, I like that better.
> 
> For R1 that doesn't matter much anyway, so we could just leave those 
> methods out completely now (and provide a large enough amount of 
> reserved virtuals). But since we want to declare that API beta 
> anyway, 
> we don't even have to provide the final answer now.

Yes, (binary) compatibility is not an issue.

> The way I think of partition updating for R2 is that the real work is 
> done by a userland app - the kernel will just have to be updated 
> about 
> the current state, it would then maintain its internal translation 
> map 
> so that the system will continue to work during those changes. That 
> translation map will also be written to disk (logged), so that the on
> -
> disk representation can always be translated correctly.
> The boot script could check for any outstanding partition changes and 
> run the DriveSetup utility if necessary.
>
> To cut a long story short - I still think it makes sense to provide 
> complete userland add-ons to implement the functionality found in the 
> different partitioning systems.

Oh, that is new. Well, in fact it is old, viz. exactly as in R5. I 
thought we were agreeing, that a separation of concerns makes sense. 
I.e. the kernel modules/add-ons would implement the complete read and 
write access, while the userland add-ons merely provide functionality 
needed for the GUI.

I actually don't see the benefit of keeping the write support out of 
the kernel. It seems even more natural to me to let the kernel do all 
the business, instead of having a userland application notify it about 
the progress -- that's an unnecessary dependency IMO.

> > > class BPartition {
> > > public:
> > >   BEmptySpace* EmptySpaceAt(int32 index) const;
> > >
> > > // I have resize and move separated because move could be 
> > > filesystem
> > > // independent, whereas resize could not
> > I hope, you're right regarding move. The absolute addressing you 
> > found in
> > ISO9660 disk scares me a bit. Maybe we can ignore that.
> 
> Well, it depends on us if we want to support resizing/moving of file 
> systems that require absolute block addressing. It would sure be 
> possible, but also much more complicated to do. Doing that online 
> (while the partition can still be accessed) might be very complicated 
> (for both, the kernel and the file system).
> The only file system that require this feature is iso9660 - and 
> there, 
> resizing/moving doesn't make any sense. But then, I don't have any 
> idea 
> about UDF (BTW what's the relation with Mount Rainier if any=3D3F). If 
> that 
> would require absolute blocks, we should go back to the drawing board 
> :)

If UDF does, I would vote for omitting support for it out of principle 
(if necessary, locking Tyler into some basement without computers). ;-)

> > >   bool CanResize() const;
> > >   bool CanMove() const;
> > >   status=3D5Ft ValidateResize(off=3D5Ft*) const;
> > >   status=3D5Ft ValidateMove(off=3D5Ft*) const;
> > >   status=3D5Ft Resize(off=3D5Ft);
> > >   status=3D5Ft Move(off=3D5Ft);
> > >
> > >   bool CanEditParameters() const;
> > >   status=3D5Ft GetParameterEditor(
> > >            BDiskScannerParameterEditor **editor,
> > >            BDiskScannerParameterEditor **parentEditor);
> > >
> > > // Perhaps we should have something for checking a partition is
> > > // large/small enough for initialization with a given system=3D3F 
> > > The
> > > // parameter editor could alternately check for this.
> > I would just add another method in BDiskSystem.
> 
> We could just add another file system error code for the 
> fs=3D5Finitialize() function.

That's too late. In DriveSetup the GUI element for selecting the system 
shall only show the feasible options. So, it must be known before 
fs=5Finitialze() is called.

> Or, we add another function to the FS API that specifically checks 
> for 
> this condition (something like "check partition" or "is partition 
> suitable" or whatever).

That's a better. Alternately the userland FS add-on (or rather the 
respective BDiskSystem) could provide a hook for it.

> > > public:
> > >   int32 CommitUpdates(BMessenger progressMessenger);
> > > };
> > status=3D5Ft CommitUpdates(bool synchronously =3D3D true,
> >     BMessenger progressMessenger =3D3D BMessenger(),
> >     BMessage *template =3D3D NULL);
> > 
> > :-)
> 
> Let's see how that will turn out with my "adding a job later" idea :)

Not too well, I suppose.

> > > class BDiskSystem {
> > > public:
> > >   bool SupportsParentSystem(const char *system) const;
> > >           // True in most cases. NULL =3D3D=3D3D raw device.
> > >   bool SupportsChildSystem(const char *system) const;
> > >           // False for most file systems, true for most partitioning
> > >           // systems.
> > >
> > >   bool IsPartitioningSystem() const;
> > >   bool IsFileSystem() const;
> > > };
> > As said above, I would make them virtual. Moreover, the Supports*
> > System()
> > should also have a BPartition* parameter. Otherwise partitioning 
> > systems
> > that allow only a limited number of levels can't give an 
> > authoritative
> > answer here. The BPartition* parameter could optionally be NULL to 
> > ask
> > whether it is supported in general.
> 
> I think there should be read-only set of classes, and an additional 
> add
> -on based set to actually do any changes. This will be only rarely 
> used, so it probably doesn't make much sense to clobber the general 
> API 
> with it - so I like something BDiskSystem (as a virtual (and 
> abstract) 
> base class), but it should contain everything, and not only the check 
> operations.
> Thoughts=3D3F

You mean that e.g. BPartition::Move() would be ommitted in favor of a 
BDiskSystem::Move(BPartition*,...)=3F
Mmh, that doesn't sound very convenient.

> > Furthermore there should be:
> > 
> >   virtual const char *Name() const;
> > 
> >   virtual bool SupportsResizing(BPartition *partition) const;
> >   virtual bool SupportsResizingChild(BPartition *child) const;
> >   virtual bool SupportsMoving(BPartition *partition) const;
> >   virtual bool SupportsMovingChild(BPartition *child) const;
> > 
> >   virtual bool ValidateResize(BPartition*, off=3D5Ft*) const;
> >   virtual bool ValidateMove(BPartition*, off=3D5Ft*) const;
> >   virtual bool ValidateResizeChild(BPartition*, off=3D5Ft*) const;
> >   virtual bool ValidateMoveChild(BPartition*, off=3D5Ft*) const;
> > 
> >   virtual bool ValidateCreateChild(BPartition*, off=3D5Ft*, off=3D5Ft*) 
> > const;
> > 
> >   virtual bool ValidateInitialize(BPartition*) const;
> 
> Looks good. I guess the Validate*() methods should change the values 
> as 
> needed=3D3F

Yes.

> So the user moves the partition to a particular location, and the 
> ValidateMove() call will change the values so that that action can 
> really be done (and the GUI can be updated directly).

That's the plan.

> > > class BDiskSystemList {
> > > public:
> > >   // To be similar to BDiskDeviceList, offering updates as
> > >   // systems are added and removed.
> > > };
> > >
> > > class BDiskDeviceRoster {
> > > public:
> > >   status=3D5Ft GetDiskSystems(BDiskSystemList *list) const;
> > > };
> > 
> > I think, I would prefer iteration methods instead (GetNext*(), 
> > Rewind
> > *()).
> > Otherwise all add-ons would need to be in memory.
> 
> That's also a reason why I want to separate reading and writing in 
> the 
> API. The kernel should always be able to read all partitioning 
> systems, 
> and therefore needs all modules loaded that handle the partitions in 
> a 
> system.

I don't get that argument. Unless we drop the userland add-ons 
completely -- which we can't since some entity needs to provide the GUI 
for editing system specific parameters -- there's always the problem, 
that they must be loaded at some point. And regardless of whether we 
put the write support into separate classes or not, that point is the 
same. Namely, when you iterate through the list of available systems.

CU, Ingo

Other related posts: