Add init reference count to all block devices #7648

davidsaada · 2018-07-30T16:45:18Z

Description

This PR fixes the problem of multiple calls to BD init/deinit APIs, by simply adding an init ref count. This is required due to the fact that utility block devices, which include underlying block devices, call the underlying init/deinit APIs automatically.
This PR partially resolves #7455 (for all BDs included in mbed-os). External BDs should be handled in a different PR.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

geky · 2018-07-30T21:06:38Z

features/filesystem/bd/ObservingBlockDevice.cpp

+    if (_init_ref_count++) {
+        return BD_ERROR_OK;
+    }
+


@davidsaada, I think I'm missing something. Is this needed if we can expect the underlying block device to also be reference counted?

Yes it is. If the block device has a resource (like BufferedBlockDevice) it would make sense to protect it against the scenario described in the issue.
Now, while it is not needed in devices like ObservingBlockDevice, ones not having any allocated resources, it is harmless, and it will become effective if one decides to add such a resource in the future. This is why I think it's a good practice to have this in all block devices, also for reference sake for future ones (which may need it).

davidsaada · 2018-07-31T08:47:13Z

@yossi2le

0xc0170 · 2018-07-31T08:51:10Z

features/filesystem/bd/BufferedBlockDevice.cpp

@@ -36,6 +36,10 @@ BufferedBlockDevice::~BufferedBlockDevice()

 int BufferedBlockDevice::init()
 {
+    if (_init_ref_count++) {


ref_count operations do not need to be atomic?

Absolutely right. Fixed now.

geky · 2018-07-31T14:20:22Z

Is this needed if we can expect the underlying block device to also be reference counted?

Now, while it is not needed in devices like ObservingBlockDevice, ones not having any allocated resources, it is harmless, and it will become effective if one decides to add such a resource in the future. This is why I think it's a good practice to have this in all block devices, also for reference sake for future ones (which may need it).

That's fair, but would a comment convey the same info without the extra code cost?

geky

Sorry I didn't see the above issue earlier, though I believe it's an easy fix.

geky · 2018-07-31T14:30:16Z

features/filesystem/bd/BlockDevice.h

@@ -217,6 +218,10 @@ class BlockDevice
            size % get_erase_size() == 0 &&
            addr + size <= this->size());
    }
+
+protected:
+    uint32_t _init_ref_count;


Sorry I didn't see this earlier. We should not have state in interfaces classes.

This adds a RAM cost to every block device class. Even ones with their own form of synchronization (ie SD, SPIF, others outside of tree).

At worst this can discourage users from applying the interface to their own code. Interfaces work best when a class satisfies as many interfaces as possible, allowing code reuse for functions that use that interface. If there's extra state in an interface, users may not adopt the interface because of the code cost. Even if it makes sense otherwise.

This is what happened with the FileLike/FileSystemLike classes. State was introduced for constructing the file tree, which on paper was a good idea, but now the interface is avoided and the poorer-named-but-stateless FileHandle class is used instead. (We still have to put up with this because of backwards compatibility, but it was a lesson learned.)

I believe this can be moved into each of the implementing classes without any cost?

Will move. Certainly now that not all classes implement it.

Thanks. The FileLike/FileSystemLike classes have been frustrating, and I didn't want to see us repeat it.

davidsaada · 2018-07-31T14:37:57Z

That's fair, but would a comment convey the same info without the extra code cost?

Fair enough. Will replace it with a comment in the resource-less classes (in the the spirit of the current efforts to reduce code size). Would suggest these include Observing, Profiling, ReadOnly & slicing.

davidsaada · 2018-07-31T16:44:43Z

Pushed fixes conforming to comments given by @geky.

cmonr · 2018-07-31T17:07:26Z

@geky Mind re-reviewing?

geky

Looks great to me 👍

Thanks for taking care of my complaints

cmonr · 2018-07-31T18:13:49Z

/morph build

cmonr · 2018-07-31T18:21:07Z

@ARMmbed/mbed-os-maintainers Targeting this for a patch even though a parameter has been added to block device constructors. Guidance: https://github.com/ARMmbed/mbed_os_release_notes/pull/7/files#diff-895497eee5018cac7ab732faa7d51128R33

mbed-ci · 2018-07-31T22:03:14Z

Build : SUCCESS

Build number : 2713
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7648/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-01T00:23:05Z

Exporter Build : FAILURE

Build number : 2342
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7648/

0xc0170 · 2018-08-01T08:19:32Z

@ARMmbed/mbed-os-maintainers Targeting this for a patch even though a parameter has been added to block device constructors.

just a private member that gets initial value (can't be set via public API anyhow). This breaks ABI (fine for any release - recompilation needed)

0xc0170 · 2018-08-01T09:31:02Z

/morph export-build

mbed-ci · 2018-08-01T10:57:47Z

Exporter Build : SUCCESS

Build number : 2346
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7648/

mbed-ci · 2018-08-01T12:44:16Z

Test : SUCCESS

Build number : 2444
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7648/2444

Add init reference count to all block devices

davidsaada mentioned this pull request Jul 30, 2018

init/deinit conflict among utility block devices on the same SD card #7455

Closed

davidsaada force-pushed the david_init_ref_count branch from 0821855 to 71fd582 Compare July 30, 2018 16:52

cmonr requested a review from a team July 30, 2018 17:00

cmonr added the needs: review label Jul 30, 2018

geky reviewed Jul 30, 2018

View reviewed changes

0xc0170 reviewed Jul 31, 2018

View reviewed changes

davidsaada force-pushed the david_init_ref_count branch 2 times, most recently from 044a52b to 75afa44 Compare July 31, 2018 12:24

geky suggested changes Jul 31, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jul 31, 2018

Add init reference count to all block devices

235f175

davidsaada force-pushed the david_init_ref_count branch from 75afa44 to 235f175 Compare July 31, 2018 16:42

cmonr added needs: review and removed needs: work labels Jul 31, 2018

geky approved these changes Jul 31, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.9.5 and removed needs: review labels Jul 31, 2018

cmonr approved these changes Jul 31, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Aug 1, 2018

0xc0170 merged commit d65e614 into ARMmbed:master Aug 1, 2018

0xc0170 removed the ready for merge label Aug 1, 2018

davidsaada deleted the david_init_ref_count branch August 1, 2018 13:25

MarceloSalazar mentioned this pull request Aug 15, 2018

Concerns with filesystem/block device in the same SD card PelionIoT/simple-mbed-cloud-client#13

Closed

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7648 from davidsaada/david_init_ref_count

fd6e912

Add init reference count to all block devices

Add init reference count to all block devices #7648

Add init reference count to all block devices #7648

Uh oh!

Conversation

davidsaada commented Jul 30, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsaada commented Jul 31, 2018

Uh oh!

0xc0170 Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsaada commented Jul 31, 2018

Uh oh!

davidsaada commented Jul 31, 2018

Uh oh!

cmonr commented Jul 31, 2018

Uh oh!

geky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jul 31, 2018

Uh oh!

cmonr commented Jul 31, 2018

Uh oh!

mbed-ci commented Jul 31, 2018

Triggering tests

Uh oh!

mbed-ci commented Aug 1, 2018

Uh oh!

0xc0170 commented Aug 1, 2018

Uh oh!

0xc0170 commented Aug 1, 2018

Uh oh!

mbed-ci commented Aug 1, 2018

Uh oh!

mbed-ci commented Aug 1, 2018

Uh oh!

Uh oh!

0xc0170 Jul 31, 2018 •

edited

Loading

geky commented Jul 31, 2018 •

edited

Loading

geky left a comment •

edited

Loading