1188230 - [CloudStorage] Cloud Storage Fundamental infrastructure on FirefoxOS

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect, P1)

Description

[Eden Chuang[:edenchuang]]

This bug is used to commit the fundamental infrastructure for cloud storage support on FirefoxOS. The detail about cloud storage support, please referring following links.

https://wiki.mozilla.org/Firefox_OS/Cloud_Storage
https://docs.google.com/presentation/d/1KX8umvvh9hnS-EeHSx1MH0kFmBocyOVmDAvccAe0pJA/edit#slide=id.g59d26e401_3_25

The fundamental infrastructure includes following parts
1. Cloud storage management in Gecko
    Cloud storage management contains few classes which used to manage and maintain the cloud storage status.
    "CloudStorage" is the class used to save information of the cloud storage. For example, storage name and mount point
    "CloudStorageManager" is the class used to manage "CloudStorage". It provides the interface to control the cloud storage for other components and a container to manage cloud storage.
    "CloudStorageRequestHandler" inherits nsRunnable class, which is used to monitor and handle the request from FUSE device. For each new cloud storage, there is a corresponding CloudStorageRequestHandler running on a new thread.

2. Volume system integration
    For each cloud storage, a corresponding fake "Volume" will be constructed and integrated into "VolumeManager". Therefore, apps can access the fake "Volume" through DeviceStorageAPI.

3. FUSE request handling and error handling
    This part is the detail implementation of CloudStorageRequestHandler. It includes following parts.
    FUSE device mounting/unmounting.
    FUSE device request monitor.
    And Fuse device request handling.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: ClassDiagram.png

The attached is the class diagram of the infrastructure.

CloudStorageManager
  The object be used to manage cloud storage and provide interface to other component.
  It contains a nsTArray to save cloud storage objects and provides following function to manage the objects.
  nsTArray::size_type NumCloudStorages();
  already_AddRefed<CloudStorage> GetCloudStorage(nsTArray::index_type aIndex);
  already_AddRefed<CloudStorage> FindCloudStorageByName(const nsCSubstring& aName);
  already_AddRefed<CloudStorage> FindAddCloudStorageByName(const nsCSubstring& aName);
  bool StartCloudStorage(const nsCSubstring& aName);
  bool StopCloudStorage(const nsCSubstring& aName);

CloudStorage
  The CloudStorage is the object used to represent corresponding cloud storage in gecko. It currently saves following information of cloud storage
  Cloud storage name
  Cloud storage mount point
  and a Cloud storage status (STATE_READY or STATE_RUNNING)
  In the constructor, it creates the mount point according to the given cloud storage name. (Current is /data/cloud/<cloudstorage_name>)
  It also provides StartStorage()/StopStorage() functions to enable/disable cloud storage feature.
  Once StartStorage() is called, the status becomes STATE_RUNNING and a new nsThread and a CloudStorageRequestHandler are created. CloudStorageRequestHandler will be submitted to the new thread to mount FUSE device and monitor the coming requests on the FUSE device.
  When StoopStorage() is called. the status becomes STATE_READY and the handler thread stop monitoring and then going to stop.

CloudStorageRequestHandler
  CloudStorageRequestHandler inherits nsRunnable and has following responsibilities
  1. Setup the FUSE device
  2. Monitor the FUSE device requests
  3. Handle the FUSE device requests
  In its constructor, it setups the FUSE device and mounts it to the corresponding mount point. And when it is destructed, it unmounts the FUSE device.
  In Run(), VolumeManager::FindAddVolumeByName() is called to create a new volume for the cloud storage, and the Volume will be set as a fake volume. Then the HandleRequests() function is called to monitor and handle the FUSE requests. HandleRequests() is a while loop which enters loop when the state is STATE_RUNNING. It handles only one request in each iteration. It also has a timeout for monitoring. Once timeout is reached, it will check the CloudStorage status. if the status is STATE_RUNNING, it restarts to monitor and handle a request again. Once the status becomes STATE_READY, the VolumeManager::RemoveVolumeByName() is called to break the combination of cloud storage and volume system.

Comment 2

[Dave Hylands [:dhylands]]

The VolumeManager is ONLY for managing volumes which are handled through Vold. And VolumeManager and Volume are designed to only be called/used from the IOThread.

So I don't think there should be any interactions between the CloudStorageRequest Handler and VolumeManager or Volume.

There is a main thread service called VolumeService and nsVolume. The fake volumes are managed here.
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIVolumeService.idl

Don't you also need to do something about usernames and passwords? Most cloud storage services I've seen require this. Or maybe this is dealt with elsewhere.

Comment 3

[Eden Chuang[:edenchuang]]

(In reply to Dave Hylands [:dhylands] from comment #2)
> The VolumeManager is ONLY for managing volumes which are handled through
> Vold. And VolumeManager and Volume are designed to only be called/used from
> the IOThread.
> 
> So I don't think there should be any interactions between the
> CloudStorageRequestHandler and VolumeManager or Volume.
> 
> There is a main thread service called VolumeService and nsVolume. The fake
> volumes are managed here.
> https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> nsIVolumeService.idl

Thanks for the suggestion.
I will use nsVolume and nsVolumeService::UpdateVolume to replace calling VolumeManager::FindAddVolumeByName for creating the corresponding fake volume.

> 
> Don't you also need to do something about usernames and passwords? Most
> cloud storage services I've seen require this. Or maybe this is dealt with
> elsewhere.

Usernames and passwords is dealt in other place. In brief, I prefer let third-party storage apps or add-ons handle the usernames, passwords or login related issues.

In fact, CloudStorageRequestHandler would not process the FUSE requests, it transfers the requests to third-party storage app/add-on through the developing WebAPI which current named FileSystemProvider API. This mechanism is called storage plug-in system. The benefit of storage plug-in system is while the cloud storage public API is upgraded, we don't need to update FirefoxOS to adapt the change, we only need upgrade the third-party storage apps/addons. However, the account management related issues is still in discussion. If we need to save username and password information in Gecko, I will add the new attributes in CloudStorage and try to resolve the related problems.

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Eden Chuang[:edenchuang] from comment #3)
> (In reply to Dave Hylands [:dhylands] from comment #2)
> > The VolumeManager is ONLY for managing volumes which are handled through
> > Vold. And VolumeManager and Volume are designed to only be called/used from
> > the IOThread.
> > 
> > So I don't think there should be any interactions between the
> > CloudStorageRequestHandler and VolumeManager or Volume.
> > 
> > There is a main thread service called VolumeService and nsVolume. The fake
> > volumes are managed here.
> > https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> > nsIVolumeService.idl
> 
> Thanks for the suggestion.
> I will use nsVolume and nsVolumeService::UpdateVolume to replace calling
> VolumeManager::FindAddVolumeByName for creating the corresponding fake
> volume.

UpdateVolume is used for syncing VolumeManager Volumes to NsVolumeService nsVolumes.

You want to use nsVolumeService::createFakeVolume and removeFakeVolume

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: class diagram of cloud storage management in Gecko

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: bug1188230-part1_cloud_storage_management_in_Gecko

Hello Dave

According to the above discussion, I implemented a patch for the cloud storage management in Gecko. Cloud you take a look and give some comments? Thanks.

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8643542 [details] [diff] [review]
bug1188230-part1_cloud_storage_management_in_Gecko

Review of attachment 8643542 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/cloudstorage/CloudStorage.cpp
@@ +28,5 @@
> +{
> +}
> +
> +void
> +CloudStorage::CreateMountPoint()

Shouldn't this function return some type of failure code to indicate whether it was successful in creating the mount point?

@@ +29,5 @@
> +}
> +
> +void
> +CloudStorage::CreateMountPoint()
> +{

What thread does this run on? We shouldn't do any I/O on the main thread.

So this function should have an assert: MOZ_ASSERT(!NS_IsMainThread());

@@ +30,5 @@
> +
> +void
> +CloudStorage::CreateMountPoint()
> +{
> +  mMountPoint.Append(MOUNTROOT);

Shouldn't this assign mMountPoint? Otherwise if you call this function twice in a row it doesn't seem like it will work properly.

@@ +33,5 @@
> +{
> +  mMountPoint.Append(MOUNTROOT);
> +  if ( -1 == mkdir(mMountPoint.get(), S_IRWXU|S_IRWXG)) {
> +    switch (errno) {
> +      case EEXIST: LOG("%s existed.", mMountPoint.get()); break;

Isn't this going to be a normal situation? We shouldn't log that the directory already exists when we expect it to.

@@ +37,5 @@
> +      case EEXIST: LOG("%s existed.", mMountPoint.get()); break;
> +      case ENOTDIR: LOG("Parent not a directory."); break;
> +      case EACCES: LOG("Search permission is denied."); break;
> +      case EROFS: LOG("Read-only filesystem."); break;
> +      default: LOG("Create %s failed with errno: %d.", mMountPoint.get(), errno);

If the directory wasn't able to be created, why does the code continue on and try to keep making subdirectories?

@@ +54,5 @@
> +      default: LOG("Create %s failed with errno: %d.", mMountPoint.get(), errno);
> +    }
> +  } else {
> +    LOG("%s is created.", mMountPoint.get());
> +  }

In the case that the mount point already exists, this should also check to make sure that the directory is empty.

@@ +73,5 @@
> +void
> +CloudStorage::StartStorage()
> +{
> +  if (mState == CloudStorage::STATE_RUNNING)
> +    return;

nit: missing braces

@@ +92,5 @@
> +void
> +CloudStorage::StopStorage()
> +{
> +  if (mState == CloudStorage::STATE_READY)
> +    return;

nit: missing braces

::: dom/system/gonk/cloudstorage/CloudStorageManager.cpp
@@ +25,5 @@
> +{
> +  if (!sCloudStorageManager) {
> +    sCloudStorageManager = new CloudStorageManager();
> +  }
> +  nsRefPtr<CloudStorageManager > cloudStorageManager

nit: extraneous space before >

@@ +41,5 @@
> +}
> +
> +already_AddRefed<CloudStorage>
> +CloudStorageManager::GetCloudStorage(size_t aIndex)
> +{

nsTArray is not thread safe.

So all of these functions which access or manipulate mCloudStorageArray need to come from the same thread (and you should assert this) or you need to provide some mutual exclusion.

::: dom/system/gonk/cloudstorage/CloudStorageRequestHandler.cpp
@@ +58,5 @@
> +      }
> +      default : {
> +        LOG("Unknown request type [%d]", mType);
> +      }
> +    } 

nit: trailing space

@@ +97,5 @@
> +  char opts[256];
> +  int res;
> +
> +  // unmount the device which mounted on the mount point.
> +  umount2(mCloudStorage->MountPoint().get(), 2);

All of the I/O calls (mount/umount/open) need to happen off the main thread.

@@ +100,5 @@
> +  // unmount the device which mounted on the mount point.
> +  umount2(mCloudStorage->MountPoint().get(), 2);
> +
> +  // open the fuse device
> +  fd = open("/dev/fuse", O_RDWR);

You need to deal with EINTR (open lists EINTR as a possible return code).

There is a MOZ_TEMP_FAILURE_RETRY macro available here: https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.h#165 which does the dirty work.

@@ +128,5 @@
> +  mFuse->next_generation = 0;
> +  mFuseHandler = new FuseHandler();
> +  mFuseHandler->fuse = mFuse;
> +  mFuseHandler->token = 0;
> +  umask(0);

You definitely can't do this. This affects the entire B2G process, and is a huge security problem.

If you need to do something with a wide open umask, then you'll need to do it in another process and use IPC to communicate with it.

@@ +176,5 @@
> +    FD_ZERO(&fds);
> +    FD_SET(mFuse->fd, &fds);
> +    struct timespec timeout;
> +    timeout.tv_sec = 0;
> +    timeout.tv_nsec = 100000000;

This is a 0.1 second timeout which seems way too small to me. I would think that it should be a big number like 10 seconds. You shouldn't need to poll here.

@@ +181,5 @@
> +
> +    int res = pselect(mFuse->fd+1, &fds, NULL, NULL, &timeout, NULL);
> +    if (res == -1 && errno != EINTR) {
> +      ERR("pselect error %d.", errno);
> +      continue;

Redoing the same operation on an error doesn't seem wise to me.

Doing a continue when errno == EINTR is appropriate and your remaining logic doesn't deal with EINTR properly (i.e. it goes ahead and treats it like a non-error)

I also don't see anyway to handle shutdown being done here. You should probably setup a pipe, and add that pipe to the select. Then add a shutdown listener which writes to the pipe to tell this thread to quit.

@@ +192,5 @@
> +        if (errno != EINTR) {
> +          ERR("[%d] handle_fuse_requests: errno=%d", mFuseHandler->token, errno);
> +        }
> +        continue;
> +      }

Same logic flaw here. The code continues on to process the data even though EINTR was received.

@@ +251,5 @@
> +    case FUSE_READDIR:
> +    case FUSE_RELEASEDIR:
> +//    case FUSE_FSYNCDIR:
> +    case FUSE_INIT: {
> +      return CLOUD_STORAGE_NO_STATUS;

Presuambly some of these need to notify device storage about files being added/removed etc.?

::: dom/system/gonk/cloudstorage/fuse.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_system_fuse_h__
> +#define mozilla_system_fuse_h__

We shouldn't be including the fuse header file here. I don't even see any version checks being done.

We should be getting the header file from the libfuse-dev package.

You'll probably need to do a toolchain update to get this installed on the build servers, but including a specific version of the fuse header here isn't appropriate.

Comment 8

[Dave Hylands [:dhylands]]

You also need to think about writing tests for this as well.

Comment 10

[Aaron Wu]

Due to resource re-arrangement, it needs more time to complete this. We decide to move to Sprint 7.

Comment 11

[BMO Automation]

Firefox OS is not being worked on