From: hch@lst.de (Christoph Hellwig)
Subject: [PATCH V2] nvmet: allow file backed ns to use cache
Date: Wed, 6 Jun 2018 14:29:39 +0200 [thread overview]
Message-ID: <20180606122939.GA13398@lst.de> (raw)
In-Reply-To: <20180531140700.13708-1-chaitanya.kulkarni@wdc.com>
> + if (ns->file && ns->buffered_io != buffered_io) {
> + nvmet_ns_disable(ns);
> + ns->buffered_io = buffered_io;
> + ret = nvmet_ns_enable(ns);
> + }
This section should be under subsys->lock to protect against
concurrent modification. For which we'd need to move the locking
out of nvmet_ns_disable/nvmet_ns_enable into the existing callers,
which should probably be a prep patch for this one.
> - int ret;
> + int flags = O_RDWR | O_LARGEFILE | ns->buffered_io ? 0 : O_DIRECT;
> struct kstat stat;
> + int ret;
Cosmetic, but I'd prefer:
int flags = O_RDWR | O_LARGEFILE;
if (!ns->buffered_io)
flags |= O_DIRECT;
>
> - ns->file = filp_open(ns->device_path,
> - O_RDWR | O_LARGEFILE | O_DIRECT, 0);
> + ns->file = filp_open(ns->device_path, flags, 0);
> @@ -97,6 +111,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
> iocb->ki_pos = pos;
> iocb->ki_filp = req->ns->file;
> iocb->ki_flags = IOCB_DIRECT | ki_flags;
> + iocb->ki_flags = ki_flags | req->ns->buffered_io ? 0 : IOCB_DIRECT;
I'd rather use iocb_flags() here, e.g.
iocb->ki_flags = ki_flags | iocb_flags(ns->file);
> static void nvmet_file_execute_flush(struct nvmet_req *req)
> {
> + if (req->ns->buffered_io)
> + flush_workqueue(req->ns->file_wq);
No needed. NVMe Flush doesn't affect in-flight commands. Quote from
the spec:
The flush applies to all commands completed prior to the submission of the
Flush command. The controller may also flush additional data and/or
metadata from any namespace.
Otherwise this looks fine.
next prev parent reply other threads:[~2018-06-06 12:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-31 14:07 [PATCH V2] nvmet: allow file backed ns to use cache Chaitanya Kulkarni
2018-06-06 12:29 ` Christoph Hellwig [this message]
[not found] ` <BN6PR04MB12037F7CBB662391A352B0CD867B0@BN6PR04MB1203.namprd04.prod.outlook.com>
2018-06-11 20:12 ` Chaitanya Kulkarni
2018-06-13 7:47 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180606122939.GA13398@lst.de \
--to=hch@lst.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox