# 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 <[email protected]>
---
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
On 08/03/2018 03:30 PM, Javier González wrote:
> # 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 <[email protected]>
> ---
> 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.
> On 4 Aug 2018, at 20.35, Matias Bjørling <[email protected]> wrote:
>
>> On 08/03/2018 03:30 PM, Javier González wrote:
>> # 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 <[email protected]>
>> ---
>> 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.
I think it’s cleaner to make a helper as the pattern repeats on all write sync I/Os. I’m ok with moving the check to the caller and only using this helper on the write path. I can send a V3 next week.
Javier.