[PATCH V2] lightnvm: pblk: take write semaphore on metadata

Subsystems: the rest

STALE2959d

3 messages, 3 authors, 2018-08-04 · open the first message on its own page

[PATCH V2] lightnvm: pblk: take write semaphore on metadata

From: Javier González <hidden>
Date: 2018-08-03 13:30:34

# Changes singe V1:
  - Fix double I/O on the read path (by Matias)
  - Improve commit message (by Jens)

pblk guarantees write ordering at a chunk level through a per open chunk
semaphore. At this point, since we only have an open I/O stream for both
user and GC data, the semaphore is per parallel unit.

Since metadata I/O is synchronous, the semaphore is not needed as
ordering is guaranteed. However, if the metadata scheme changes or
multiple streams are open, this guarantee might not be preserved.

This patch makes sure that all writes go through the semaphore, even for
synchronous I/O. This is consistent with pblk's write I/O model. It also
simplifies maintenance since changes in the metdatada scheme could cause
ordering issues.

Signed-off-by: Javier González <redacted>
---
 drivers/lightnvm/pblk-core.c | 18 ++++++++++++++++--
 drivers/lightnvm/pblk.h      |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 00984b486fea..6432faf5b19c 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -493,6 +493,20 @@ int pblk_submit_io_sync(struct pblk *pblk, struct nvm_rq *rqd)
 	return nvm_submit_io_sync(dev, rqd);
 }
 
+int pblk_submit_io_sync_sem(struct pblk *pblk, struct nvm_rq *rqd)
+{
+	int ret;
+
+	if (rqd->opcode != NVM_OP_PWRITE)
+		return pblk_submit_io_sync(pblk, rqd);
+
+	pblk_down_page(pblk, rqd->ppa_list, rqd->nr_ppas);
+	ret = pblk_submit_io_sync(pblk, rqd);
+	pblk_up_page(pblk, rqd->ppa_list, rqd->nr_ppas);
+
+	return ret;
+}
+
 static void pblk_bio_map_addr_endio(struct bio *bio)
 {
 	bio_put(bio);
@@ -737,7 +751,7 @@ static int pblk_line_submit_emeta_io(struct pblk *pblk, struct pblk_line *line,
 		}
 	}
 
-	ret = pblk_submit_io_sync(pblk, &rqd);
+	ret = pblk_submit_io_sync_sem(pblk, &rqd);
 	if (ret) {
 		pblk_err(pblk, "emeta I/O submission failed: %d\n", ret);
 		bio_put(bio);
@@ -842,7 +856,7 @@ static int pblk_line_submit_smeta_io(struct pblk *pblk, struct pblk_line *line,
 	 * the write thread is the only one sending write and erase commands,
 	 * there is no need to take the LUN semaphore.
 	 */
-	ret = pblk_submit_io_sync(pblk, &rqd);
+	ret = pblk_submit_io_sync_sem(pblk, &rqd);
 	if (ret) {
 		pblk_err(pblk, "smeta I/O submission failed: %d\n", ret);
 		bio_put(bio);
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 4760af7b6499..6ccc6ad8e1ce 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -782,6 +782,7 @@ void pblk_log_write_err(struct pblk *pblk, struct nvm_rq *rqd);
 void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd);
 int pblk_submit_io(struct pblk *pblk, struct nvm_rq *rqd);
 int pblk_submit_io_sync(struct pblk *pblk, struct nvm_rq *rqd);
+int pblk_submit_io_sync_sem(struct pblk *pblk, struct nvm_rq *rqd);
 int pblk_submit_meta_io(struct pblk *pblk, struct pblk_line *meta_line);
 struct bio *pblk_bio_map_addr(struct pblk *pblk, void *data,
 			      unsigned int nr_secs, unsigned int len,
-- 
2.7.4

Re: [PATCH V2] lightnvm: pblk: take write semaphore on metadata

From: Matias Bjørling <hidden>
Date: 2018-08-04 18:35:41

On 08/03/2018 03:30 PM, Javier González wrote:
quoted hunk
# Changes singe V1:
   - Fix double I/O on the read path (by Matias)
   - Improve commit message (by Jens)

pblk guarantees write ordering at a chunk level through a per open chunk
semaphore. At this point, since we only have an open I/O stream for both
user and GC data, the semaphore is per parallel unit.

Since metadata I/O is synchronous, the semaphore is not needed as
ordering is guaranteed. However, if the metadata scheme changes or
multiple streams are open, this guarantee might not be preserved.

This patch makes sure that all writes go through the semaphore, even for
synchronous I/O. This is consistent with pblk's write I/O model. It also
simplifies maintenance since changes in the metdatada scheme could cause
ordering issues.

Signed-off-by: Javier González <redacted>
---
  drivers/lightnvm/pblk-core.c | 18 ++++++++++++++++--
  drivers/lightnvm/pblk.h      |  1 +
  2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 00984b486fea..6432faf5b19c 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -493,6 +493,20 @@ int pblk_submit_io_sync(struct pblk *pblk, struct nvm_rq *rqd)
  	return nvm_submit_io_sync(dev, rqd);
  }
  
+int pblk_submit_io_sync_sem(struct pblk *pblk, struct nvm_rq *rqd)
Nitpicking a bit. It looks to me that a function that has semaphore in 
its name, should take the semaphore in all cases unless it returns an 
error. When it only does it on writes, it creates confusion.

Maybe this would be one of the cases where it is okay to have the logic 
it in the caller function, or do such that it takes a flag if it should 
take the semaphore. That'll make it explicit when it is done.

Re: [PATCH V2] lightnvm: pblk: take write semaphore on metadata

From: Javier Gonzalez <hidden>
Date: 2018-08-04 18:39:55

DQoNCj4gT24gNCBBdWcgMjAxOCwgYXQgMjAuMzUsIE1hdGlhcyBCasO4cmxpbmcgPG1iQGxpZ2h0
bnZtLmlvPiB3cm90ZToNCj4gDQo+PiBPbiAwOC8wMy8yMDE4IDAzOjMwIFBNLCBKYXZpZXIgR29u
esOhbGV6IHdyb3RlOg0KPj4gIyBDaGFuZ2VzIHNpbmdlIFYxOg0KPj4gICAtIEZpeCBkb3VibGUg
SS9PIG9uIHRoZSByZWFkIHBhdGggKGJ5IE1hdGlhcykNCj4+ICAgLSBJbXByb3ZlIGNvbW1pdCBt
ZXNzYWdlIChieSBKZW5zKQ0KPj4gcGJsayBndWFyYW50ZWVzIHdyaXRlIG9yZGVyaW5nIGF0IGEg
Y2h1bmsgbGV2ZWwgdGhyb3VnaCBhIHBlciBvcGVuIGNodW5rDQo+PiBzZW1hcGhvcmUuIEF0IHRo
aXMgcG9pbnQsIHNpbmNlIHdlIG9ubHkgaGF2ZSBhbiBvcGVuIEkvTyBzdHJlYW0gZm9yIGJvdGgN
Cj4+IHVzZXIgYW5kIEdDIGRhdGEsIHRoZSBzZW1hcGhvcmUgaXMgcGVyIHBhcmFsbGVsIHVuaXQu
DQo+PiBTaW5jZSBtZXRhZGF0YSBJL08gaXMgc3luY2hyb25vdXMsIHRoZSBzZW1hcGhvcmUgaXMg
bm90IG5lZWRlZCBhcw0KPj4gb3JkZXJpbmcgaXMgZ3VhcmFudGVlZC4gSG93ZXZlciwgaWYgdGhl
IG1ldGFkYXRhIHNjaGVtZSBjaGFuZ2VzIG9yDQo+PiBtdWx0aXBsZSBzdHJlYW1zIGFyZSBvcGVu
LCB0aGlzIGd1YXJhbnRlZSBtaWdodCBub3QgYmUgcHJlc2VydmVkLg0KPj4gVGhpcyBwYXRjaCBt
YWtlcyBzdXJlIHRoYXQgYWxsIHdyaXRlcyBnbyB0aHJvdWdoIHRoZSBzZW1hcGhvcmUsIGV2ZW4g
Zm9yDQo+PiBzeW5jaHJvbm91cyBJL08uIFRoaXMgaXMgY29uc2lzdGVudCB3aXRoIHBibGsncyB3
cml0ZSBJL08gbW9kZWwuIEl0IGFsc28NCj4+IHNpbXBsaWZpZXMgbWFpbnRlbmFuY2Ugc2luY2Ug
Y2hhbmdlcyBpbiB0aGUgbWV0ZGF0YWRhIHNjaGVtZSBjb3VsZCBjYXVzZQ0KPj4gb3JkZXJpbmcg
aXNzdWVzLg0KPj4gU2lnbmVkLW9mZi1ieTogSmF2aWVyIEdvbnrDoWxleiA8amF2aWVyQGNuZXhs
YWJzLmNvbT4NCj4+IC0tLQ0KPj4gIGRyaXZlcnMvbGlnaHRudm0vcGJsay1jb3JlLmMgfCAxOCAr
KysrKysrKysrKysrKysrLS0NCj4+ICBkcml2ZXJzL2xpZ2h0bnZtL3BibGsuaCAgICAgIHwgIDEg
Kw0KPj4gIDIgZmlsZXMgY2hhbmdlZCwgMTcgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkN
Cj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2xpZ2h0bnZtL3BibGstY29yZS5jIGIvZHJpdmVycy9s
aWdodG52bS9wYmxrLWNvcmUuYw0KPj4gaW5kZXggMDA5ODRiNDg2ZmVhLi42NDMyZmFmNWIxOWMg
MTAwNjQ0DQo+PiAtLS0gYS9kcml2ZXJzL2xpZ2h0bnZtL3BibGstY29yZS5jDQo+PiArKysgYi9k
cml2ZXJzL2xpZ2h0bnZtL3BibGstY29yZS5jDQo+PiBAQCAtNDkzLDYgKzQ5MywyMCBAQCBpbnQg
cGJsa19zdWJtaXRfaW9fc3luYyhzdHJ1Y3QgcGJsayAqcGJsaywgc3RydWN0IG52bV9ycSAqcnFk
KQ0KPj4gICAgICByZXR1cm4gbnZtX3N1Ym1pdF9pb19zeW5jKGRldiwgcnFkKTsNCj4+ICB9DQo+
PiAgK2ludCBwYmxrX3N1Ym1pdF9pb19zeW5jX3NlbShzdHJ1Y3QgcGJsayAqcGJsaywgc3RydWN0
IG52bV9ycSAqcnFkKQ0KPiANCj4gTml0cGlja2luZyBhIGJpdC4gSXQgbG9va3MgdG8gbWUgdGhh
dCBhIGZ1bmN0aW9uIHRoYXQgaGFzIHNlbWFwaG9yZSBpbiBpdHMgbmFtZSwgc2hvdWxkIHRha2Ug
dGhlIHNlbWFwaG9yZSBpbiBhbGwgY2FzZXMgdW5sZXNzIGl0IHJldHVybnMgYW4gZXJyb3IuIFdo
ZW4gaXQgb25seSBkb2VzIGl0IG9uIHdyaXRlcywgaXQgY3JlYXRlcyBjb25mdXNpb24uDQo+IA0K
PiBNYXliZSB0aGlzIHdvdWxkIGJlIG9uZSBvZiB0aGUgY2FzZXMgd2hlcmUgaXQgaXMgb2theSB0
byBoYXZlIHRoZSBsb2dpYyBpdCBpbiB0aGUgY2FsbGVyIGZ1bmN0aW9uLCBvciBkbyBzdWNoIHRo
YXQgaXQgdGFrZXMgYSBmbGFnIGlmIGl0IHNob3VsZCB0YWtlIHRoZSBzZW1hcGhvcmUuIFRoYXQn
bGwgbWFrZSBpdCBleHBsaWNpdCB3aGVuIGl0IGlzIGRvbmUuDQoNCkkgdGhpbmsgaXTigJlzIGNs
ZWFuZXIgdG8gbWFrZSBhIGhlbHBlciBhcyB0aGUgcGF0dGVybiByZXBlYXRzIG9uIGFsbCB3cml0
ZSBzeW5jIEkvT3MuIEnigJltIG9rIHdpdGggbW92aW5nIHRoZSBjaGVjayB0byB0aGUgY2FsbGVy
IGFuZCBvbmx5IHVzaW5nIHRoaXMgaGVscGVyIG9uIHRoZSB3cml0ZSBwYXRoLiBJIGNhbiBzZW5k
IGEgVjMgbmV4dCB3ZWVrLiANCg0KSmF2aWVyLiA=
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help