Linux-NVME Archive on lore.kernel.org
 help / color / mirror / Atom feed
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.

  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