ext2: Fix enabling/disabling the file cache

* Inode:
  - Rename {Enable,Disable}FileCache() to {Create,Delete}FileCache()
    and IsFileCacheDisabled() to HasFileCache(), since that is what they
    actually do. DeleteFileCache() now also sets the attributes to NULL,
    which makes fCached superfluous.
  - Introduce {Enable,Disable}FileCache(), which actually enable/disable
    the file cache. Use those methods for handling O_NOCACHE.
* ext2_free_cookie(): Reenable the file cache in case of O_NOCACHE.

Fixes crash when O_NOCACHE was used, since the file cache was deleted
without clearing the attribute and Inode::ReadAt() would use the deleted
object afterward.

src/add-ons/kernel/file_systems/ext2/Inode.cpp

@@ -37,7 +37,6 @@ Inode::Inode(Volume* volume, ino_t id)
 	fID(id),
 	fCache(NULL),
 	fMap(NULL),
-	fCached(false),
 	fHasExtraAttributes(false)
 {
 	rw_lock_init(&fLock, "ext2 inode");
@@ -56,11 +55,9 @@ Inode::Inode(Volume* volume, ino_t id)
 
 		if (IsDirectory() || (IsSymLink() && Size() < 60)) {
 			TRACE("Inode::Inode(): Not creating the file cache\n");
-			fCached = false;
-
 			fInitStatus = B_OK;
 		} else
-			fInitStatus = EnableFileCache();
+			fInitStatus = CreateFileCache();
 	} else
 		TRACE("Inode: Failed initialization\n");
 }
@@ -72,7 +69,6 @@ Inode::Inode(Volume* volume)
 	fID(0),
 	fCache(NULL),
 	fMap(NULL),
-	fCached(false),
 	fInitStatus(B_NO_INIT)
 {
 	rw_lock_init(&fLock, "ext2 inode");
@@ -89,11 +85,7 @@ Inode::~Inode()
 {
 	TRACE("Inode destructor\n");
 
-	if (fCached) {
+	DeleteFileCache();
-		TRACE("Deleting the file cache and file map\n");
-		file_cache_delete(FileCache());
-		file_map_delete(Map());
-	}
 
 	TRACE("Inode destructor: Done\n");
 }
@@ -269,7 +261,7 @@ Inode::WriteAt(Transaction& transaction, off_t pos, const uint8* buffer,
 		buffer, _length, *_length);
 	ReadLocker readLocker(fLock);
 
-	if (IsFileCacheDisabled())
+	if (!HasFileCache())
 		return B_BAD_VALUE;
 
 	if (pos < 0)
@@ -697,7 +689,7 @@ Inode::Create(Transaction& transaction, Inode* parent, const char* name,
 	if (!inode->IsSymLink()) {
 		// Vnode::Publish doesn't publish symlinks
 		if (!inode->IsDirectory()) {
-			status = inode->EnableFileCache();
+			status = inode->CreateFileCache();
 			if (status != B_OK)
 				return status;
 		}
@@ -726,31 +718,59 @@ Inode::Create(Transaction& transaction, Inode* parent, const char* name,
 
 
 status_t
-Inode::EnableFileCache()
+Inode::CreateFileCache()
 {
-	TRACE("Inode::EnableFileCache()\n");
+	TRACE("Inode::CreateFileCache()\n");
 
-	if (fCached)
+	if (fCache != NULL)
 		return B_OK;
-	if (fCache != NULL) {
-		fCached = true;
-		return B_OK;
-	}
 
-	TRACE("Inode::EnableFileCache(): Creating file cache: %" B_PRIu32 ", %"
+	TRACE("Inode::CreateFileCache(): Creating file cache: %" B_PRIu32 ", %"
 		B_PRIdINO ", %" B_PRIdOFF "\n", fVolume->ID(), ID(), Size());
-	fCache = file_cache_create(fVolume->ID(), ID(), Size());
-	fMap = file_map_create(fVolume->ID(), ID(), Size());
 
+	fCache = file_cache_create(fVolume->ID(), ID(), Size());
 	if (fCache == NULL) {
-		ERROR("Inode::EnableFileCache(): Failed to create file cache\n");
+		ERROR("Inode::CreateFileCache(): Failed to create file cache\n");
-		fCached = false;
 		return B_ERROR;
 	}
 
-	fCached = true;
+	fMap = file_map_create(fVolume->ID(), ID(), Size());
-	TRACE("Inode::EnableFileCache(): Done\n");
+	if (fMap == NULL) {
+		ERROR("Inode::CreateFileCache(): Failed to create file map\n");
+		file_cache_delete(fCache);
+		fCache = NULL;
+		return B_ERROR;
+	}
 
+	TRACE("Inode::CreateFileCache(): Done\n");
+
+	return B_OK;
+}
+
+
+void
+Inode::DeleteFileCache()
+{
+	TRACE("Inode::DeleteFileCache()\n");
+
+	if (fCache == NULL)
+		return;
+
+	file_cache_delete(fCache);
+	file_map_delete(fMap);
+
+	fCache = NULL;
+	fMap = NULL;
+}
+
+
+status_t
+Inode::EnableFileCache()
+{
+	if (fCache == NULL)
+		return B_BAD_VALUE;
+
+	file_cache_enable(fCache);
 	return B_OK;
 }
 
@@ -758,15 +778,9 @@ Inode::EnableFileCache()
 status_t
 Inode::DisableFileCache()
 {
-	TRACE("Inode::DisableFileCache()\n");
+	status_t error = file_cache_disable(fCache);
-
+	if (error != B_OK)
-	if (!fCached)
+		return error;
-		return B_OK;
-
-	file_cache_delete(FileCache());
-	file_map_delete(Map());
-
-	fCached = false;
 
 	return B_OK;
 }
@@ -775,7 +789,7 @@ Inode::DisableFileCache()
 status_t
 Inode::Sync()
 {
-	if (!IsFileCacheDisabled())
+	if (HasFileCache())
 		return file_cache_sync(fCache);
 
 	return B_OK;

src/add-ons/kernel/file_systems/ext2/Inode.h

@@ -112,9 +112,11 @@ public:
 	
 			void*		FileCache() const { return fCache; }
 			void*		Map() const { return fMap; }
+			status_t	CreateFileCache();
+			void		DeleteFileCache();
+			bool		HasFileCache() { return fCache != NULL; }
 			status_t	EnableFileCache();
 			status_t	DisableFileCache();
-			bool		IsFileCacheDisabled() const { return !fCached; }
 
 			status_t	Sync();
 
@@ -144,7 +146,6 @@ private:
 			ino_t		fID;
 			void*		fCache;
 			void*		fMap;
-			bool		fCached;
 			bool		fUnlinked;
 			bool		fHasExtraAttributes;
 			ext2_inode	fNode;

src/add-ons/kernel/file_systems/ext2/InodeJournal.cpp

@@ -39,12 +39,11 @@ InodeJournal::InodeJournal(Inode* inode)
 		fJournalVolume = volume;
 		fJournalBlockCache = volume->BlockCache();
 
-		if (!inode->IsFileCacheDisabled())
+		if (inode->HasFileCache())
-			fInitStatus = inode->DisableFileCache();
+			inode->DeleteFileCache();
-		else
+
 		fInitStatus = B_OK;
 
-		if (fInitStatus == B_OK) {
 		TRACE("InodeJournal::InodeJournal(): Inode's file cache disabled "
 			"successfully\n");
 		HashRevokeManager* revokeManager = new(std::nothrow)
@@ -66,7 +65,6 @@ InodeJournal::InodeJournal(Inode* inode)
 				delete revokeManager;
 		}
 	}
-	}
 }
 
 

src/add-ons/kernel/file_systems/ext2/kernel_interface.cpp

@@ -759,7 +759,7 @@ ext2_create(fs_volume* _volume, fs_vnode* _directory, const char* name,
 
 	TRACE("ext2_create(): Created inode\n");
 
-	if ((openMode & O_NOCACHE) != 0 && !inode->IsFileCacheDisabled()) {
+	if ((openMode & O_NOCACHE) != 0) {
 		status = inode->DisableFileCache();
 		if (status != B_OK)
 			return status;
@@ -830,8 +830,8 @@ ext2_create_symlink(fs_volume* _volume, fs_vnode* _directory, const char* name,
 			link->Mode(), 0);
 		put_vnode(volume->FSVolume(), id);
 
-		if (link->IsFileCacheDisabled()) {
+		if (!link->HasFileCache()) {
-			status = link->EnableFileCache();
+			status = link->CreateFileCache();
 			if (status != B_OK)
 				return status;
 		}
@@ -1132,10 +1132,12 @@ ext2_open(fs_volume* _volume, fs_vnode* _node, int openMode, void** _cookie)
 	cookie->last_size = inode->Size();
 	cookie->last_notification = system_time();
 
+	MethodDeleter<Inode, status_t> fileCacheEnabler(&Inode::EnableFileCache);
 	if ((openMode & O_NOCACHE) != 0) {
 		status = inode->DisableFileCache();
 		if (status != B_OK)
 			return status;
+		fileCacheEnabler.SetTo(inode);
 	}
 
 	// Should we truncate the file?
@@ -1157,6 +1159,7 @@ ext2_open(fs_volume* _volume, fs_vnode* _node, int openMode, void** _cookie)
 		// TODO: No need to notify file size changed?
 	}
 
+	fileCacheEnabler.Detach();
 	cookieDeleter.Detach();
 	*_cookie = cookie;
 
@@ -1246,6 +1249,9 @@ ext2_free_cookie(fs_volume* _volume, fs_vnode* _node, void* _cookie)
 	if (inode->Size() != cookie->last_size)
 		notify_stat_changed(volume->ID(), inode->ID(), B_STAT_SIZE);
 
+	if ((cookie->open_mode & O_NOCACHE) != 0)
+		inode->EnableFileCache();
+
 	delete cookie;
 	return B_OK;
 }