From f0d93820475083e456ce56c978bcb2b2cc80edd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Sun, 11 Nov 2007 13:07:56 +0000 Subject: [PATCH] * The previous locking order "inode write" -> "journal" was not only not honoured (in the Inode attribute code), it also couldn't be honoured there. Therefore, the locking order is now reversed, that is "journal" comes first, then the Inode write lock. This should also help with the lock ups that appeared after r22886. * Got rid of INODE_NO_TRANSACTION; it doesn't make any sense to use. * When Inode::WriteAttribute() changed the type, the type field was actually never written back within the current transaction. * If the attribute data fit into the inode again, the attribute's write lock was never released. * Since r22886, Inode::ReadAt() does its own locking, so we must no longer lock the inode before calling it. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@22897 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- src/add-ons/kernel/file_systems/bfs/Inode.cpp | 73 +++++++++---------- src/add-ons/kernel/file_systems/bfs/Lock.h | 15 +++- src/add-ons/kernel/file_systems/bfs/bfs.h | 1 - .../file_systems/bfs/kernel_interface.cpp | 58 ++++++++------- 4 files changed, 81 insertions(+), 66 deletions(-) diff --git a/src/add-ons/kernel/file_systems/bfs/Inode.cpp b/src/add-ons/kernel/file_systems/bfs/Inode.cpp index 4664fbfc45..7e0ddd673b 100644 --- a/src/add-ons/kernel/file_systems/bfs/Inode.cpp +++ b/src/add-ons/kernel/file_systems/bfs/Inode.cpp @@ -55,11 +55,13 @@ InodeAllocator::~InodeAllocator() status_t -InodeAllocator::New(block_run *parentRun, mode_t mode, block_run &run, Inode **_inode) +InodeAllocator::New(block_run *parentRun, mode_t mode, block_run &run, + Inode **_inode) { Volume *volume = fTransaction->GetVolume(); - status_t status = volume->AllocateForInode(*fTransaction, parentRun, mode, fRun); + status_t status = volume->AllocateForInode(*fTransaction, parentRun, mode, + fRun); if (status < B_OK) { // don't free the space in the destructor, because // the allocation failed @@ -101,7 +103,8 @@ InodeAllocator::CreateTree() if (fInode->IsRegularNode()) { if (tree->Insert(*fTransaction, ".", fInode->ID()) < B_OK - || tree->Insert(*fTransaction, "..", volume->ToVnode(fInode->Parent())) < B_OK) + || tree->Insert(*fTransaction, "..", + volume->ToVnode(fInode->Parent())) < B_OK) return B_ERROR; } return B_OK; @@ -833,11 +836,7 @@ Inode::ReadAttribute(const char *name, int32 type, off_t pos, uint8 *buffer, Inode *attribute; status_t status = GetAttribute(name, &attribute); if (status == B_OK) { - if (attribute->Lock().Lock() == B_OK) { - status = attribute->ReadAt(pos, (uint8 *)buffer, _length); - attribute->Lock().Unlock(); - } else - status = B_ERROR; + status = attribute->ReadAt(pos, (uint8 *)buffer, _length); ReleaseAttribute(attribute); } @@ -862,6 +861,8 @@ Inode::WriteAttribute(Transaction &transaction, const char *name, int32 type, // TODO: we actually depend on that the contents of "buffer" are constant. // If they get changed during the write (hey, user programs), we may mess // up our index trees! + // TODO: for attribute files, we need to log the first BPLUSTREE_MAX_KEY_LENGTH + // bytes of the data stream, or the same as above might happen. Index index(fVolume); index.SetTo(name); @@ -916,19 +917,24 @@ Inode::WriteAttribute(Transaction &transaction, const char *name, int32 type, // check if the data fits into the small_data section again NodeGetter node(fVolume, transaction, this); - status = _AddSmallData(transaction, node, name, type, buffer, *_length); + status = _AddSmallData(transaction, node, name, type, buffer, + *_length); + if (status == B_OK) { // it does - remove its file + attribute->Lock().UnlockWrite(); status = _RemoveAttribute(transaction, name, false, NULL); } else { - status = attribute->WriteAt(transaction, pos, buffer, _length); - if (status == B_OK) { - // The attribute type might have been changed - we need to adopt - // the new one - the attribute's inode will be written back on close - attribute->Node().type = HOST_ENDIAN_TO_BFS_INT32(type); - } - + // The attribute type might have been changed - we need to + // adopt the new one + attribute->Node().type = HOST_ENDIAN_TO_BFS_INT32(type); + status = attribute->WriteBack(transaction); attribute->Lock().UnlockWrite(); + + if (status == B_OK) { + status = attribute->WriteAt(transaction, pos, buffer, + _length); + } } } else status = B_ERROR; @@ -976,8 +982,8 @@ Inode::RemoveAttribute(Transaction &transaction, const char *name) uint32 length = smallData->DataSize(); if (length > BPLUSTREE_MAX_KEY_LENGTH) length = BPLUSTREE_MAX_KEY_LENGTH; - index.Update(transaction, name, smallData->Type(), smallData->Data(), - length, NULL, 0, this); + index.Update(transaction, name, smallData->Type(), + smallData->Data(), length, NULL, 0, this); } fSmallDataLock.Unlock(); } @@ -1224,14 +1230,6 @@ Inode::FindBlockRun(off_t pos, block_run &run, off_t &offset) status_t Inode::ReadAt(off_t pos, uint8 *buffer, size_t *_length) { -#if 0 - // call the right ReadAt() method, depending on the inode flags - - if (Flags() & INODE_LOGGED) - return ((Stream *)this)->ReadAt(pos, buffer, _length); - - return ((Stream *)this)->ReadAt(pos, buffer, _length); -#endif size_t length = *_length; // set/check boundaries for pos/length @@ -1266,27 +1264,28 @@ Inode::WriteAt(Transaction &transaction, off_t pos, const uint8 *buffer, << INODE_TIME_SHIFT); // TODO: support INODE_LOGGED! -#if 0 - if (Flags() & INODE_LOGGED) - return ((Stream *)this)->WriteAt(transaction, pos, buffer, _length); - return ((Stream *)this)->WriteAt(transaction, pos, buffer, _length); -#endif size_t length = *_length; + bool changeSize = false; // set/check boundaries for pos/length if (pos < 0) return B_BAD_VALUE; + if (pos + length > Size()) + changeSize = true; + + locker.Unlock(); + + // the transaction doesn't have to be started already + if (changeSize && !transaction.IsStarted()) + transaction.Start(fVolume, BlockNumber()); + + locker.Lock(); + if (pos + length > Size()) { off_t oldSize = Size(); - // the transaction doesn't have to be started already - // ToDo: what's that INODE_NO_TRANSACTION flag good for again? - if ((Flags() & INODE_NO_TRANSACTION) == 0 - && !transaction.IsStarted()) - transaction.Start(fVolume, BlockNumber()); - // let's grow the data stream to the size needed status_t status = SetFileSize(transaction, pos + length); if (status < B_OK) { diff --git a/src/add-ons/kernel/file_systems/bfs/Lock.h b/src/add-ons/kernel/file_systems/bfs/Lock.h index bf65640b99..7626bbbe92 100644 --- a/src/add-ons/kernel/file_systems/bfs/Lock.h +++ b/src/add-ons/kernel/file_systems/bfs/Lock.h @@ -479,7 +479,7 @@ class WriteLocked { : fLock(lock) { - fStatus = lock != NULL ? lock->LockWrite() : B_ERROR; + fStatus = lock != NULL ? lock->LockWrite() : B_NO_INIT; } ~WriteLocked() @@ -488,6 +488,14 @@ class WriteLocked { fLock->UnlockWrite(); } + status_t + Lock() + { + if (fStatus == B_OK) + return B_ERROR; + return fStatus = fLock->LockWrite(); + } + status_t IsLocked() { @@ -497,8 +505,9 @@ class WriteLocked { void Unlock() { - fLock->UnlockWrite(); - fStatus = B_NO_INIT; + if (fStatus == B_OK) + fLock->UnlockWrite(); + fStatus = B_ERROR; } private: diff --git a/src/add-ons/kernel/file_systems/bfs/bfs.h b/src/add-ons/kernel/file_systems/bfs/bfs.h index 23e2d95823..b7a7c2990f 100644 --- a/src/add-ons/kernel/file_systems/bfs/bfs.h +++ b/src/add-ons/kernel/file_systems/bfs/bfs.h @@ -217,7 +217,6 @@ enum inode_flags { INODE_PERMANENT_FLAGS = 0x0000ffff, INODE_WAS_WRITTEN = 0x00020000, - INODE_NO_TRANSACTION = 0x00040000, INODE_DONT_FREE_SPACE = 0x00080000, // only used by the "chkbfs" functionality INODE_CHKBFS_RUNNING = 0x00200000, }; diff --git a/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp b/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp index 125eb4ca83..ff7f4df2fa 100644 --- a/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp +++ b/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp @@ -694,20 +694,19 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask) Volume *volume = (Volume *)_ns; Inode *inode = (Inode *)_node; - // that may be incorrect here - I don't think we need write access to - // change most of the stat... - // we should definitely check a bit more if the new stats are correct and valid... + // TODO: we should definitely check a bit more if the new stats are + // valid - or even better, the VFS should check this before calling us status_t status = inode->CheckPermissions(W_OK); if (status < B_OK) RETURN_ERROR(status); + Transaction transaction(volume, inode->BlockNumber()); + WriteLocked locked(inode->Lock()); if (locked.IsLocked() < B_OK) RETURN_ERROR(B_ERROR); - Transaction transaction(volume, inode->BlockNumber()); - bfs_inode &node = inode->Node(); if (mask & B_STAT_SIZE) { @@ -737,7 +736,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask) if (mask & B_STAT_MODE) { PRINT(("original mode = %ld, stat->st_mode = %d\n", node.Mode(), stat->st_mode)); - node.mode = HOST_ENDIAN_TO_BFS_INT32(node.Mode() & ~S_IUMSK | stat->st_mode & S_IUMSK); + node.mode = HOST_ENDIAN_TO_BFS_INT32(node.Mode() & ~S_IUMSK + | stat->st_mode & S_IUMSK); } if (mask & B_STAT_UID) @@ -762,7 +762,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask) (bigtime_t)stat->st_crtime << INODE_TIME_SHIFT); } - if ((status = inode->WriteBack(transaction)) == B_OK) + status = inode->WriteBack(transaction); + if (status == B_OK) transaction.Done(); notify_stat_changed(volume->ID(), inode->ID(), mask); @@ -772,8 +773,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask) status_t -bfs_create(void *_ns, void *_directory, const char *name, int openMode, int mode, - void **_cookie, ino_t *_vnodeID) +bfs_create(void *_ns, void *_directory, const char *name, int openMode, + int mode, void **_cookie, ino_t *_vnodeID) { FUNCTION_START(("name = \"%s\", perms = %d, openMode = %d\n", name, mode, openMode)); @@ -1147,8 +1148,8 @@ bfs_open(void *_fs, void *_node, int openMode, void **_cookie) // Should we truncate the file? if (openMode & O_TRUNC) { - WriteLocked locked(inode->Lock()); Transaction transaction(volume, inode->BlockNumber()); + WriteLocked locked(inode->Lock()); status_t status = inode->SetFileSize(transaction, 0); if (status >= B_OK) @@ -1255,25 +1256,32 @@ bfs_free_cookie(void *_ns, void *_node, void *_cookie) return B_BAD_VALUE; file_cookie *cookie = (file_cookie *)_cookie; - Volume *volume = (Volume *)_ns; Inode *inode = (Inode *)_node; - WriteLocked locked(inode->Lock()); + Transaction transaction; + bool needsTrimming; - bool needsTrimming = inode->NeedsTrimming(); + { + ReadLocked locker(inode->Lock()); + needsTrimming = inode->NeedsTrimming(); - if ((cookie->open_mode & O_RWMASK) != 0 - && !inode->IsDeleted() - && (needsTrimming - || inode->OldLastModified() != inode->LastModified() - || inode->OldSize() != inode->Size())) { + if ((cookie->open_mode & O_RWMASK) != 0 + && !inode->IsDeleted() + && (needsTrimming + || inode->OldLastModified() != inode->LastModified() + || inode->OldSize() != inode->Size())) { + locker.Unlock(); + transaction.Start(volume, inode->BlockNumber()); + } + } + WriteLocked locker(inode->Lock()); + status_t status = B_ERROR; + + if (transaction.IsStarted()) { // trim the preallocated blocks and update the size, // and last_modified indices if needed - - Transaction transaction(volume, inode->BlockNumber()); - status_t status = B_OK; bool changedSize = false, changedTime = false; Index index(volume); @@ -1299,14 +1307,14 @@ bfs_free_cookie(void *_ns, void *_node, void *_cookie) inode->WriteBack(transaction); } - if (status == B_OK) - transaction.Done(); - if (changedSize || changedTime) { notify_stat_changed(volume->ID(), inode->ID(), - (changedTime ? B_STAT_MODIFICATION_TIME : 0) | (changedSize ? B_STAT_SIZE : 0)); + (changedTime ? B_STAT_MODIFICATION_TIME : 0) + | (changedSize ? B_STAT_SIZE : 0)); } } + if (status == B_OK) + transaction.Done(); if (inode->Flags() & INODE_CHKBFS_RUNNING) { // "chkbfs" exited abnormally, so we have to stop it here...