Make writeback cache default (#887)

* stream data validation

* stream data validation

* Enable writeback caching safely

* Flipped boolean semantics for writeback cache

* Added CLI param

* Add debug log for write only in case it is a problem

* Modified test to work with enable writeback cache

* Added a flag to ignore append flag

* github comments

* Added tests for ignore append

* changelog

* Added fuse2 test

* correcting file mode when ignore-append-file flag is true

* fix unit test

* Changes for ignore open flags

* test for libfuse2

* add ignore open flag to all config

* Updated README

* Added to Changelog

* Quick test only in data validation for streaming

Co-authored-by: Tamer Sherif <[email protected]>
Co-authored-by: vibhansa-msft <[email protected]>
Co-authored-by: souravgupta <[email protected]>

CHANGELOG.md

@@ -4,7 +4,9 @@
 - Added support for displaying mount space utilization based on file cache consumption (for example when doing `df`)
 - Added support for updating MD5 sum on file upload
 - Added support for validating MD5 sum on download
-- Added support to enable writeback cache for read only workloads and user configurable for read-write workloads. 
+- Added backwards compatibility support for all blobfuse v1 CLI options
+- Added support to allow disabling writeback cache if a customer is opening a file with O_APPEND
+- Added support to ignore append flag on open when writeback cache is on
 
 **Bug Fixes**
 - Fixed a bug in parsing output of disk utilization summary

README.md

@@ -113,6 +113,8 @@ To learn about a specific command, just include the name of the command (For exa
     * `--entry-timeout=<TIMEOUT IN SECONDS>`: Time the kernel can cache directory listing.
     * `--negative-timeout=<TIMEOUT IN SECONDS>`: Time the kernel can cache non-existance of file or directory.
     * `--allow-other`: Allow other users to have access this mount point.
+    * `--disable-writeback-cache`: Disallow libfuse to buffer write requests if you must strictly open files in O_WRONLY or O_APPEND mode.
+    * `--ignore-open-flags`: Ignore the append and write only flag since O_APPEND and O_WRONLY is not supported with writeback caching.
 
 
 ## Environment variables
@@ -149,6 +151,8 @@ To learn about a specific command, just include the name of the command (For exa
 - How do I generate a SAS with permissions for rename?
 az cli has a command to generate a sas token. Open a command prompt and make sure you are logged in to az cli. Run the following command and the sas token will be displayed in the command prompt.
 az storage container generate-sas --account-name <account name ex:myadlsaccount> --account-key <accountKey> -n <container name> --permissions dlrwac --start <today's date ex: 2021-03-26> --expiry <date greater than the current time ex:2021-03-28>
+- Why do I get EINVAL on opening a file with WRONLY or APPEND flags?
+To improve performance, Blobfuse2 by default enables writeback caching, which can produce unexpected behavior for files opened with WRONLY or APPEND flags, so Blobfuse2 returns EINVAL on open of a file with those flags. Either use disable-writeback-caching to turn off writeback caching (can potentiallu result in degraded performance) or ignore-open-flags (replace WRONLY with RDWR and ignore APPEND) based on your workload. 
 
 ## Un-Supported File system operations
 - mkfifo : fifo creation is not supported by blobfuse2 and this will result in "function not implemented" error
@@ -167,7 +171,7 @@ az storage container generate-sas --account-name <account name ex:myadlsaccount>
   for details on this.
 
 ## Limitations
-- In case of BlockBlob accounts, ACLs are not supported by Azure Storage so Blobfuse2 will bydefault return success for 'chmod' operation. However it will work fine for Gen2 (DataLake) accounts.
+- In case of BlockBlob accounts, ACLs are not supported by Azure Storage so Blobfuse2 will by default return success for 'chmod' operation. However it will work fine for Gen2 (DataLake) accounts.
 
 
 ### Syslog security warning

azure-pipeline-templates/distro-tests.yml

@@ -177,7 +177,7 @@ steps:
         adls: false
         idstring: 'Distro BlockBlob with Stream Key Credentials'
         distro_name: ${{ parameters.distro_name }}
-        quick_test: ${{ parameters.quick_test }}
+        quick_test: true
         artifact_name: '${{ parameters.distro_name }}_block_stream_key.txt'
         verbose_log: ${{ parameters.verbose_log }}
         clone: false

component/libfuse/libfuse.go

@@ -54,20 +54,21 @@ import (
 // Common structure for Component
 type Libfuse struct {
 	internal.BaseComponent
-	mountPath            string
-	dirPermission        uint
-	filePermission       uint
-	readOnly             bool
-	attributeExpiration  uint32
-	entryExpiration      uint32
-	negativeTimeout      uint32
-	allowOther           bool
-	ownerUID             uint32
-	ownerGID             uint32
-	traceEnable          bool
-	extensionPath        string
-	enableWritebackCache bool
-	lsFlags              common.BitMap16
+	mountPath             string
+	dirPermission         uint
+	filePermission        uint
+	readOnly              bool
+	attributeExpiration   uint32
+	entryExpiration       uint32
+	negativeTimeout       uint32
+	allowOther            bool
+	ownerUID              uint32
+	ownerGID              uint32
+	traceEnable           bool
+	extensionPath         string
+	disableWritebackCache bool
+	ignoreOpenFlag        bool
+	lsFlags               common.BitMap16
 }
 
 // To support pagination in readdir calls this structure holds a block of items for a given directory
@@ -90,7 +91,8 @@ type LibfuseOptions struct {
 	allowOther              bool   `config:"allow-other"`
 	readOnly                bool   `config:"read-only"`
 	ExtensionPath           string `config:"extension" yaml:"extension,omitempty"`
-	EnableWritebackCache    bool   `config:"enable-writeback-cache"`
+	DisableWritebackCache   bool   `config:"disable-writeback-cache"`
+	IgnoreOpenFlag          bool   `config:"ignore-open-flag"`
 }
 
 const compName = "libfuse"
@@ -165,8 +167,8 @@ func (lf *Libfuse) Validate(opt *LibfuseOptions) error {
 	lf.traceEnable = opt.EnableFuseTrace
 	lf.allowOther = opt.allowOther
 	lf.extensionPath = opt.ExtensionPath
-	// Setting here to make testing easier.
-	lf.enableWritebackCache = opt.readOnly || opt.EnableWritebackCache
+	lf.disableWritebackCache = opt.DisableWritebackCache
+	lf.ignoreOpenFlag = opt.IgnoreOpenFlag
 
 	if opt.allowOther {
 		lf.dirPermission = uint(common.DefaultAllowOtherPermissionBits)
@@ -246,10 +248,6 @@ func (lf *Libfuse) Configure(_ bool) error {
 		return fmt.Errorf("config error in %s [invalid config settings]", lf.Name())
 	}
 
-	if config.IsSet(compName + ".ignore-open-flags") {
-		log.Warn("unsupported v1 CLI parameter: ignore-open-flags is always true in blobfuse2.")
-	}
-
 	log.Info("Libfuse::Configure : read-only %t, allow-other %t, default-perm %d, entry-timeout %d, attr-time %d, negative-timeout %d",
 		lf.readOnly, lf.allowOther, lf.filePermission, lf.entryExpiration, lf.attributeExpiration, lf.negativeTimeout)
 
@@ -286,11 +284,13 @@ func init() {
 	allowOther := config.AddBoolFlag("allow-other", false, "Allow other users to access this mount point.")
 	config.BindPFlag("allow-other", allowOther)
 
+	disableWritebackCache := config.AddBoolFlag("disable-writeback-cache", false, "Disallow libfuse to buffer write requests if you must strictly open files in O_WRONLY or O_APPEND mode.")
+	config.BindPFlag(compName+".disable-writeback-cache", disableWritebackCache)
+
 	debug := config.AddBoolPFlag("d", false, "Mount with foreground and FUSE logs on.")
 	config.BindPFlag(compName+".fuse-trace", debug)
 	debug.Hidden = true
 
-	ignoreOpenFlags := config.AddBoolFlag("ignore-open-flags", false, "Ignore unsupported open flags by blobfuse.")
+	ignoreOpenFlags := config.AddBoolFlag("ignore-open-flags", false, "Ignore unsupported open flags (APPEND, WRONLY) by blobfuse when writeback caching is enabled.")
 	config.BindPFlag(compName+".ignore-open-flags", ignoreOpenFlags)
-	ignoreOpenFlags.Hidden = true
 }

component/libfuse/libfuse2_handler_test_wrapper.go

@@ -239,9 +239,9 @@ func testOpenSyncDirectFlag(suite *libfuseTestSuite) {
 	path := C.CString("/" + name)
 	defer C.free(unsafe.Pointer(path))
 	mode := fs.FileMode(fuseFS.filePermission)
-	flags := C.O_RDWR & C.O_SYNC & C.__O_DIRECT & 0xffffffff
+	flags := C.O_RDWR & 0xffffffff
 	info := &C.fuse_file_info_t{}
-	info.flags = C.O_RDWR & C.O_SYNC & C.__O_DIRECT
+	info.flags = C.O_RDWR | C.O_SYNC | C.__O_DIRECT
 	options := internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
 	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
 
@@ -251,6 +251,102 @@ func testOpenSyncDirectFlag(suite *libfuseTestSuite) {
 	suite.assert.Equal(C.int(0), info.flags&C.__O_DIRECT)
 }
 
+// fuse2 does not have writeback caching, so append flag is passed unchanged
+func testOpenAppendFlagDefault(suite *libfuseTestSuite) {
+	defer suite.cleanupTest()
+
+	name := "path"
+	path := C.CString("/" + name)
+	defer C.free(unsafe.Pointer(path))
+	mode := fs.FileMode(fuseFS.filePermission)
+	flags := C.O_RDWR | C.O_APPEND&0xffffffff
+	info := &C.fuse_file_info_t{}
+	info.flags = C.O_RDWR | C.O_APPEND
+	options := internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err := libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+
+	flags = C.O_WRONLY | C.O_APPEND&0xffffffff
+	info = &C.fuse_file_info_t{}
+	info.flags = C.O_WRONLY | C.O_APPEND
+	options = internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err = libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+}
+
+func testOpenAppendFlagDisableWritebackCache(suite *libfuseTestSuite) {
+	defer suite.cleanupTest()
+	suite.cleanupTest() // clean up the default libfuse generated
+	config := "libfuse:\n  disable-writeback-cache: true\n"
+	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
+	suite.assert.True(suite.libfuse.disableWritebackCache)
+
+	name := "path"
+	path := C.CString("/" + name)
+	defer C.free(unsafe.Pointer(path))
+	mode := fs.FileMode(fuseFS.filePermission)
+	flags := C.O_RDWR | C.O_APPEND&0xffffffff
+	info := &C.fuse_file_info_t{}
+	info.flags = C.O_RDWR | C.O_APPEND
+	options := internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err := libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+
+	flags = C.O_WRONLY | C.O_APPEND&0xffffffff
+	info = &C.fuse_file_info_t{}
+	info.flags = C.O_WRONLY | C.O_APPEND
+	options = internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err = libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+}
+
+func testOpenAppendFlagIgnoreAppendFlag(suite *libfuseTestSuite) {
+	defer suite.cleanupTest()
+	suite.cleanupTest() // clean up the default libfuse generated
+	config := "libfuse:\n  ignore-open-flag: true\n"
+	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
+	suite.assert.True(suite.libfuse.ignoreOpenFlag)
+
+	name := "path"
+	path := C.CString("/" + name)
+	defer C.free(unsafe.Pointer(path))
+	mode := fs.FileMode(fuseFS.filePermission)
+	flags := C.O_RDWR | C.O_APPEND&0xffffffff
+	info := &C.fuse_file_info_t{}
+	info.flags = C.O_RDWR | C.O_APPEND
+	options := internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err := libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+
+	flags = C.O_WRONLY | C.O_APPEND&0xffffffff
+	info = &C.fuse_file_info_t{}
+	info.flags = C.O_WRONLY | C.O_APPEND
+	options = internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err = libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+
+	flags = C.O_WRONLY & 0xffffffff
+	info = &C.fuse_file_info_t{}
+	info.flags = C.O_WRONLY
+	options = internal.OpenFileOptions{Name: name, Flags: flags, Mode: mode}
+	suite.mock.EXPECT().OpenFile(options).Return(&handlemap.Handle{}, nil)
+
+	err = libfuse_open(path, info)
+	suite.assert.Equal(C.int(0), err)
+}
+
 func testOpenNotExists(suite *libfuseTestSuite) {
 	defer suite.cleanupTest()
 	name := "path"

component/libfuse/libfuse_handler.go

@@ -279,10 +279,9 @@ func libfuse_init(conn *C.fuse_conn_info_t, cfg *C.fuse_config_t) (res unsafe.Po
 		FUSE_CAP_WRITEBACK_CACHE flag is not suitable for network filesystems.  If a partial page is
 		written, then the page needs to be first read from userspace.  This means, that
 		even for files opened for O_WRONLY it is possible that READ requests will be
-		generated by the kernel. This will result in error in file cache
+		generated by the kernel.
 	*/
-	enable_writeback_cache := C.bool(fuseFS.enableWritebackCache)
-	if enable_writeback_cache && ((conn.capable & C.FUSE_CAP_WRITEBACK_CACHE) != 0) {
+	if !fuseFS.disableWritebackCache && ((conn.capable & C.FUSE_CAP_WRITEBACK_CACHE) != 0) {
 		// Buffer write requests at libfuse and then hand it off to application
 		log.Info("Libfuse::libfuse_init : Enable Capability : FUSE_CAP_WRITEBACK_CACHE")
 		conn.want |= C.FUSE_CAP_WRITEBACK_CACHE
@@ -612,12 +611,25 @@ func libfuse_open(path *C.char, fi *C.fuse_file_info_t) C.int {
 	if fi.flags&C.O_SYNC != 0 || fi.flags&C.__O_DIRECT != 0 {
 		log.Err("Libfuse::libfuse_open : Reset flags for open %s, fi.flags %X", name, fi.flags)
 		// Blobfuse2 does not support the SYNC or DIRECT flag. If a user application passes this flag on to blobfuse2
-		// and we open the file with this flag, subsequent write operations wlil fail with "Invalid argument" error.
+		// and we open the file with this flag, subsequent write operations will fail with "Invalid argument" error.
 		// Mask them out here in the open call so that write works.
 		// Oracle RMAN is one such application that sends these flags during backup
 		fi.flags = fi.flags &^ C.O_SYNC
 		fi.flags = fi.flags &^ C.__O_DIRECT
 	}
+	if !fuseFS.disableWritebackCache {
+		if fi.flags&C.O_ACCMODE == C.O_WRONLY || fi.flags&C.O_APPEND != 0 {
+			if fuseFS.ignoreOpenFlag {
+				log.Warn("Libfuse::libfuse_open : Flags (%X) not supported to open %s when write back cache is on. Ignoring unsupported flags.", fi.flags, name)
+				// O_ACCMODE disables both RDONLY, WRONLY and RDWR flags
+				fi.flags = fi.flags &^ (C.O_APPEND | C.O_ACCMODE)
+				fi.flags = fi.flags | C.O_RDWR
+			} else {
+				log.Err("Libfuse::libfuse_open : Flag (%X) not supported to open %s when write back cache is on. Pass --disable-writeback-cache or --ignore-open-flag via CLI", fi.flags, name)
+				return -C.EINVAL
+			}
+		}
+	}
 
 	handle, err := fuseFS.NextComponent().OpenFile(
 		internal.OpenFileOptions{

component/libfuse/libfuse_handler_test.go

@@ -55,20 +55,22 @@ func (suite *libfuseTestSuite) TestDefault() {
 	suite.assert.Equal(suite.libfuse.entryExpiration, uint32(120))
 	suite.assert.Equal(suite.libfuse.attributeExpiration, uint32(120))
 	suite.assert.Equal(suite.libfuse.negativeTimeout, uint32(120))
-	suite.assert.False(suite.libfuse.enableWritebackCache)
+	suite.assert.False(suite.libfuse.disableWritebackCache)
+	suite.assert.False(suite.libfuse.ignoreOpenFlag)
 }
 
 func (suite *libfuseTestSuite) TestConfig() {
 	defer suite.cleanupTest()
 	suite.cleanupTest() // clean up the default libfuse generated
-	config := "allow-other: true\nread-only: true\nlibfuse:\n  attribute-expiration-sec: 60\n  entry-expiration-sec: 60\n  negative-entry-expiration-sec: 60\n  fuse-trace: true\n  enable-writeback-cache: true\n"
+	config := "allow-other: true\nread-only: true\nlibfuse:\n  attribute-expiration-sec: 60\n  entry-expiration-sec: 60\n  negative-entry-expiration-sec: 60\n  fuse-trace: true\n  disable-writeback-cache: true\n  ignore-open-flag: true\n"
 	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
 
 	suite.assert.Equal(suite.libfuse.Name(), "libfuse")
 	suite.assert.Empty(suite.libfuse.mountPath)
 	suite.assert.True(suite.libfuse.readOnly)
 	suite.assert.True(suite.libfuse.traceEnable)
-	suite.assert.True(suite.libfuse.enableWritebackCache)
+	suite.assert.True(suite.libfuse.disableWritebackCache)
+	suite.assert.True(suite.libfuse.ignoreOpenFlag)
 	suite.assert.True(suite.libfuse.allowOther)
 	suite.assert.Equal(suite.libfuse.dirPermission, uint(fs.FileMode(0777)))
 	suite.assert.Equal(suite.libfuse.filePermission, uint(fs.FileMode(0777)))
@@ -113,28 +115,34 @@ func (suite *libfuseTestSuite) TestConfigDefaultPermission() {
 	suite.assert.Equal(suite.libfuse.negativeTimeout, uint32(0))
 }
 
-func (suite *libfuseTestSuite) TestEnableWritebackCache() {
+func (suite *libfuseTestSuite) TestDisableWritebackCache() {
 	defer suite.cleanupTest()
-	// Default ro: false, ewc: false
-	suite.assert.False(suite.libfuse.enableWritebackCache)
+	suite.assert.False(suite.libfuse.disableWritebackCache)
 
-	// ro: false, ewc: true
 	suite.cleanupTest() // clean up the default libfuse generated
-	config := "read-only: false\nlibfuse:\n  enable-writeback-cache: true\n"
+	config := "libfuse:\n  disable-writeback-cache: true\n"
 	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
-	suite.assert.True(suite.libfuse.enableWritebackCache)
+	suite.assert.True(suite.libfuse.disableWritebackCache)
 
-	// ro: true, ewc: false
 	suite.cleanupTest() // clean up the default libfuse generated
-	config = "read-only: true\nlibfuse:\n  enable-writeback-cache: false\n"
+	config = "libfuse:\n  disable-writeback-cache: false\n"
 	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
-	suite.assert.True(suite.libfuse.enableWritebackCache)
+	suite.assert.False(suite.libfuse.disableWritebackCache)
+}
+
+func (suite *libfuseTestSuite) TestIgnoreAppendFlag() {
+	defer suite.cleanupTest()
+	suite.assert.False(suite.libfuse.ignoreOpenFlag)
+
+	suite.cleanupTest() // clean up the default libfuse generated
+	config := "libfuse:\n  ignore-open-flag: true\n"
+	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
+	suite.assert.True(suite.libfuse.ignoreOpenFlag)
 
-	// ro: true, ewc: true
 	suite.cleanupTest() // clean up the default libfuse generated
-	config = "read-only: true\nlibfuse:\n  enable-writeback-cache: true\n"
+	config = "libfuse:\n  ignore-open-flag: false\n"
 	suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
-	suite.assert.True(suite.libfuse.enableWritebackCache)
+	suite.assert.False(suite.libfuse.ignoreOpenFlag)
 }
 
 // getattr
@@ -173,10 +181,22 @@ func (suite *libfuseTestSuite) TestOpen() {
 	testOpen(suite)
 }
 
-func (suite *libfuseTestSuite) TestOpenSyncFlag() {
+func (suite *libfuseTestSuite) TestOpenSyncDirectFlag() {
 	testOpenSyncDirectFlag(suite)
 }
 
+func (suite *libfuseTestSuite) TestOpenAppendFlagDefault() {
+	testOpenAppendFlagDefault(suite)
+}
+
+func (suite *libfuseTestSuite) TestOpenAppendFlagDisableWritebackCache() {
+	testOpenAppendFlagDisableWritebackCache(suite)
+}
+
+func (suite *libfuseTestSuite) TestOpenAppendFlagIgnoreAppendFlag() {
+	testOpenAppendFlagIgnoreAppendFlag(suite)
+}
+
 func (suite *libfuseTestSuite) TestOpenNotExists() {
 	testOpenNotExists(suite)
 }