[haiku-commits] Re: BRANCH ahenriksson-github.production - src/add-ons/kernel/file_systems/bfs
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Thu, 12 Jul 2012 21:01:42 +0200
On 10.07.2012 20:49, ahenriksson-github.production wrote:
+ // these can't fail if AllocateBlocks works correctly, but
perhaps
+ // they ought to be checked in the release build anyways rather
than
+ // risking user data? (since we're not actually using the
returned run)
+ ASSERT(allocatedRun.AllocationGroup() == 0);
+ ASSERT(allocatedRun.Start() == oldEnd);
+ ASSERT(allocatedRun.Length() == allocationSize);
If it doesn't work correctly, the data is likely to be toasted, anyway :-)
IMO this is the perfect use for an ASSERT.
+ // lock journal here?
+
+ status = FlushLogAndBlocks();
+ if (status != B_OK)
+ return status;
+
+ status = recursive_lock_lock(&fLock);
Yes, you should lock the journal first if your intention is to have a
clean log at this point (as it should).
BTW instead of calling recursive_lock_lock() manually, you might want to
use a RecursiveLocker instead -- less error prone, and nicer to read, too.
+ status = fVolume->WriteSuperBlock();
+ if (status != B_OK) {
+ fVolume->SuperBlock().log_blocks = oldLog;
+ recursive_lock_unlock(&fLock);
+
+ /*
+ // if we had to allocate some blocks, try to free them
+ // TODO: does it even make sense to try this after we couldn't
write
+ // the superblock?
+ if (!allocatedRun.IsZero()) {
It's unlikely to succeed, but why not. Otherwise the next checkfs run
would free the log as well.
status_t
+BlockAllocator::Reinitialize()
+{
+ RecursiveLocker locker(fLock);
+
+ // need to write back any pending changes to the block bitmap
+ // TODO: shall we read through the cache in _Initialize instead?
+ status_t status = fVolume->GetJournal(0)->FlushLogAndBlocks();
That would make _Initialize() a bit nicer, but also more complicated.
Also, resizing the block bitmap probably cannot be done in a single
transaction, as the log might be too small, or in a transaction at all,
since the log area might interfere.
When growing a disk, a solution might be to temporary move the log to a
safe place (atomically) first, and the resize the disk.
BTW, how do you solve that the block bitmap is used, but must not
overwrite existing data at the same time?
+ off_t newLogSize = 2048;
+ if (newNumBlocks <= 20480)
+ newLogSize = 512;
+ if (newSize > 1LL * 1024 * 1024 * 1024)
+ newLogSize = 4096;
Maybe add a static method containing this logic to be used by
Initialize() as well.
+ if (newNumBlocks < GetVolume()->NumBlocks()) {
+ fShrinking = true;
+ } else {
+ fShrinking = false;
+ } // aTODO handle no resize needed / no data moving needed
If you could not fulfill the user's request, I would return a meaningful
error code like B_NOT_SUPPORTED (if it's theoretically possible), or
something like B_BAD_VALUE.
+ //if (newReservedLength
+ //> GetVolume()->Log().Start() +
GetVolume()->Log().Length()) {
+
+ Start(VISIT_REGULAR | VISIT_INDICES | VISIT_REMOVED
+ /*| VISIT_ATTRIBUTE_DIRECTORIES can't move these yet*/);
I guess that is just a matter of updating the parent?
+
+ /*
+ if (newNumBlocks < GetVolume()->NumBlocks()) {
+ // shrinking
+ //move data to allowed
+ //do resize
+ //move journal(?)
Moving the journal is always problematic, as it's the only thing that
cannot use the journal :-)
However, it could be done in an atomic way, and that certainly should be
done if possible.
+ } else if (newNumBlocks > GetVolume()->NumBlocks()) {
+ // growing
+ if (newReservedLength
+ > GetVolume()->Log().Start() +
GetVolume()->Log().Length()) {
+ //move data out of the area we need as reserved
+ }
+ //move journal
+ //do resize
How would that work in this direction? How to move the journal before
having made space for it by moving away inodes?
Or are these just leftovers? :-)
+ // if the inode is not outside the allowed area, we might be able to
+ // resize anyways
+ FATAL(("Could not open inode at %" B_PRIdOFF "\n", id));
+
+ if (fForce)
+ return B_OK;
While that is true in theory, it's probably not too much to ask for a
clean disk before resizing (ie. letting the user call checkfs in case of
errors).
+ // make sure we have space to resize the bitmap
+ if (1 + fBitmapBlocks > GetVolume()->Log().Start()) {
+ return B_ERROR;
+ }
Superfluous curly braces.
+status_t
+ResizeVisitor::_MoveInode(Inode* inode)
+{
[...]
+ status = _UpdateParent(transaction, inode, newInodeID);
+ if (status != B_OK)
+ return status;
+
+ status = _UpdateParent(transaction, inode, newInodeID);
+ if (status != B_OK)
+ return status;
Doing it twice is twice as much fun?
+status_t
+ResizeVisitor::_TemporaryGetDiskSize(off_t& size)
What does the temporary mean?
diff --git a/src/add-ons/kernel/file_systems/bfs/ResizeVisitor.h
b/src/add-ons/kernel/file_systems/bfs/ResizeVisitor.h
new file mode 100644
index 0000000..a8a93ff
--- /dev/null
+++ b/src/add-ons/kernel/file_systems/bfs/ResizeVisitor.h
@@ -0,0 +1,58 @@
+#ifndef RESIZE_VISITOR_H
+#define RESIZE_VISITOR_H
You should add a license header, too.
+
ResizeVisitor(Volume* volume) // aTODO tmp
+ :
+
FileSystemVisitor(volume),
+
fForce(false) {}
Unless it's performance critical, I would put that into the source file
instead.
diff --git a/src/add-ons/kernel/file_systems/bfs/Volume.h
b/src/add-ons/kernel/file_systems/bfs/Volume.h
index f7f3c35..729fe30 100644
@@ -121,6 +122,10 @@ public:
{ return
find_thread(NULL) == fCheckingThread; }
CheckVisitor*& GetCheckVisitor() { return
fCheckVisitor; }
I would prefer a dedicated setter for this instead, or even a
CreateCheckVisitor() method instead.
Also, the above method should be called CheckVisitor() - it can be a bit
annoying in this case, though, since the class has the same name, and
you would need to add the scope to the class name, ie. use ::CheckVisitor.
+ // resizing
+ // TODO / aTODO: do we need SetResizingThread?
aTODO? :-)
Bye,
Axel.
Other related posts:
