linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends
@ 2013-10-02 11:00 Maxim Patlasov
  2013-10-02 11:01 ` [PATCH 1/5] fuse: writepages: roll back changes if request not found Maxim Patlasov
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:00 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

Miklos,

The patch-set fixes a few issues I found while reviewing your
recent "fixes for writepages" patch-set.

The patches are for writepages.v2. If they are OK, I'll re-check
them for for-next.

Thanks,
Maxim

---

Maxim Patlasov (5):
      fuse: writepages: roll back changes if request not found
      fuse: writepages: crop secondary requests on send
      fuse: writepages: crop secondary requests on attach
      fuse: writepage: update bdi writeout when deleting secondary request
      fuse: writepages: protect secondary requests from fuse file release


 fs/fuse/file.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 12 deletions(-)

-- 
Signature

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] fuse: writepages: roll back changes if request not found
  2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
@ 2013-10-02 11:01 ` Maxim Patlasov
  2013-10-02 11:01 ` [PATCH 2/5] fuse: writepages: crop secondary requests on send Maxim Patlasov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:01 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

fuse_writepage_in_flight() returns false if it fails to find request with
given index in fi->writepages. Then the caller proceeds with populating
data->orig_pages[] and incrementing req->num_pages. Hence,
fuse_writepage_in_flight() must revert changes it made in request before
returning false.

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 135360e..575e44f 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1659,8 +1659,11 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 			break;
 		}
 	}
-	if (!found)
+	if (!found) {
+		new_req->num_pages = 0;
+		list_add(&new_req->writepages_entry, &fi->writepages);
 		goto out_unlock;
+	}
 
 	for (tmp = old_req; tmp != NULL; tmp = tmp->misc.write.next) {
 		BUG_ON(tmp->inode != new_req->inode);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] fuse: writepages: crop secondary requests on send
  2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
  2013-10-02 11:01 ` [PATCH 1/5] fuse: writepages: roll back changes if request not found Maxim Patlasov
@ 2013-10-02 11:01 ` Maxim Patlasov
  2013-10-02 11:17   ` [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2 Maxim Patlasov
  2013-10-02 11:02 ` [PATCH 3/5] fuse: writepages: crop secondary requests on attach Maxim Patlasov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:01 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

If writeback happens while fuse is in FUSE_NOWRITE condition, the request
will be queued but not processed immediately (see fuse_flush_writepages()).
Until FUSE_NOWRITE becomes relaxed, more writebacks can happen. They will
be queued as "secondary" requests to that first ("primary") request.

When FUSE_NOWRITE is relaxed and fuse_send_writepage() is called, it must
crop both primary and secondary requests according to the actual i_size.
Otherwise, if only primary is cropped, an extending write(2) may increase
i_size soon and then secondary requests won't be cropped properly. The result
would be stale data written to the server to a file offset where zeros must be.

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 575e44f..a5d1f87 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1435,6 +1435,50 @@ static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)
 	wake_up(&fi->page_waitq);
 }
 
+/* Drop list of secondary writepage requests */
+static void fuse_drop_writepage(struct fuse_conn *fc, struct fuse_req *req)
+{
+	struct backing_dev_info *bdi = req->inode->i_mapping->backing_dev_info;
+
+	while (req) {
+		struct fuse_req *next = req->misc.write.next;
+		dec_bdi_stat(bdi, BDI_WRITEBACK);
+		dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
+		fuse_writepage_free(fc, req);
+		fuse_put_request(fc, req);
+		req = next;
+	}
+}
+
+/* Crop the misc.write.in.size of parent and secondary writepage requests */
+static bool fuse_crop_writepage(struct fuse_conn *fc, struct fuse_req *req,
+				loff_t size, struct fuse_req **drop_list)
+{
+	if (req->misc.write.in.offset >= size)
+		return true;
+
+	while (req) {
+		struct fuse_req *next = req->misc.write.next;
+		struct fuse_write_in *inarg = &req->misc.write.in;
+		__u64 data_size = inarg->size ? :
+			req->num_pages * PAGE_CACHE_SIZE;
+
+		if (inarg->offset + data_size <= size) {
+			inarg->size = data_size;
+		} else if (inarg->offset < size) {
+			inarg->size = size - inarg->offset;
+		} else {
+			/* Got truncated off completely */
+			req->misc.write.next = *drop_list;
+			*drop_list = req;
+		}
+
+		req = next;
+	}
+
+	return false;
+}
+
 /* Called under fc->lock, may release and reacquire it */
 static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req)
 __releases(fc->lock)
@@ -1443,29 +1487,30 @@ __acquires(fc->lock)
 	struct fuse_inode *fi = get_fuse_inode(req->inode);
 	loff_t size = i_size_read(req->inode);
 	struct fuse_write_in *inarg = &req->misc.write.in;
-	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
+	struct fuse_req *drop_list = NULL;
 
 	if (!fc->connected)
 		goto out_free;
 
-	if (inarg->offset + data_size <= size) {
-		inarg->size = data_size;
-	} else if (inarg->offset < size) {
-		inarg->size = size - inarg->offset;
-	} else {
-		/* Got truncated off completely */
-		goto out_free;
-	}
+	if (fuse_crop_writepage(fc, req, size, &drop_list))
+		goto out_free; /* drop req and descendants */
 
 	req->in.args[1].size = inarg->size;
 	fi->writectr++;
 	fuse_request_send_background_locked(fc, req);
+
+	if (drop_list) {
+		spin_unlock(&fc->lock);
+		fuse_drop_writepage(fc, drop_list);
+		spin_lock(&fc->lock);
+	}
 	return;
 
  out_free:
 	fuse_writepage_finish(fc, req);
 	spin_unlock(&fc->lock);
 	fuse_writepage_free(fc, req);
+	fuse_drop_writepage(fc, req->misc.write.next);
 	fuse_put_request(fc, req);
 	spin_lock(&fc->lock);
 }


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/5] fuse: writepages: crop secondary requests on attach
  2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
  2013-10-02 11:01 ` [PATCH 1/5] fuse: writepages: roll back changes if request not found Maxim Patlasov
  2013-10-02 11:01 ` [PATCH 2/5] fuse: writepages: crop secondary requests on send Maxim Patlasov
@ 2013-10-02 11:02 ` Maxim Patlasov
  2013-10-02 11:02 ` [PATCH 4/5] fuse: writepage: update bdi writeout when deleting secondary request Maxim Patlasov
  2013-10-02 11:03 ` [PATCH 5/5] fuse: writepages: protect secondary requests from fuse file release Maxim Patlasov
  4 siblings, 0 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:02 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

If a request was already cropped according to i_size (i.e. went through
fuse_send_writepage()) and then another writeback happens and we decided
to attach it the request, we must crop it according to misc.write.in.size
of the request. Otherwise the following scenario could lead to writing
stale data (where zeros are expected):

1. Shrinking ftruncate(2) is handled by fuse_do_setattr() which eventually
calls fuse_release_nowrite() which properly crops requests sitting in
fi->queued_writes. Since now some range in a page-cache page is invalid
(i.e. contains stale data which shouldn't come to the server). This properly
reflected by misc.write.in.size of a request.
2. The page is re-dirtied, new writeback happens, fuse_writepage_in_flight()
attaches new request to the request (because it's already in-flight).
3. Extending ftruncate(2) increases i_size, so by the time that attached
request comes to fuse_send_writepage(), i_size is already extended
and that stale range of a page will come to the server. The result is that
the user will see stale data where zeros are expected.

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a5d1f87..036fcf3 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1680,6 +1680,14 @@ static void fuse_writepages_send(struct fuse_fill_wb_data *data)
 		end_page_writeback(data->orig_pages[i]);
 }
 
+static inline loff_t fuse_req_end_pos(struct fuse_req *req)
+{
+	__u64 data_size = req->misc.write.in.size ? :
+		req->num_pages * PAGE_CACHE_SIZE;
+
+	return req->misc.write.in.offset + data_size;
+}
+
 static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 				     struct page *page)
 {
@@ -1689,6 +1697,7 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 	struct fuse_req *old_req;
 	bool found = false;
 	pgoff_t curr_index;
+	bool page_copied = false;
 
 	BUG_ON(new_req->num_pages != 0);
 
@@ -1722,8 +1731,12 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 	if (old_req->num_pages == 1 && (old_req->state == FUSE_REQ_INIT ||
 					old_req->state == FUSE_REQ_PENDING)) {
 		copy_highpage(old_req->pages[0], page);
-		spin_unlock(&fc->lock);
+		page_copied = true;
+	}
 
+	if (page_copied ||
+	    new_req->misc.write.in.offset >= fuse_req_end_pos(old_req)) {
+		spin_unlock(&fc->lock);
 		dec_bdi_stat(page->mapping->backing_dev_info, BDI_WRITEBACK);
 		dec_zone_page_state(page, NR_WRITEBACK_TEMP);
 		fuse_writepage_free(fc, new_req);
@@ -1732,6 +1745,11 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 	} else {
 		new_req->misc.write.next = old_req->misc.write.next;
 		old_req->misc.write.next = new_req;
+
+		if (fuse_req_end_pos(new_req) > fuse_req_end_pos(old_req))
+			new_req->misc.write.in.size =
+				fuse_req_end_pos(old_req) -
+				new_req->misc.write.in.offset;
 	}
 out_unlock:
 	spin_unlock(&fc->lock);


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/5] fuse: writepage: update bdi writeout when deleting secondary request
  2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
                   ` (2 preceding siblings ...)
  2013-10-02 11:02 ` [PATCH 3/5] fuse: writepages: crop secondary requests on attach Maxim Patlasov
@ 2013-10-02 11:02 ` Maxim Patlasov
  2013-10-02 11:03 ` [PATCH 5/5] fuse: writepages: protect secondary requests from fuse file release Maxim Patlasov
  4 siblings, 0 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:02 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

BDI_WRITTEN counter is used to estimate bdi bandwidth. It must be incremented
every time as bdi ends page writeback. No matter whether it was fulfilled by
actual write or by discarding the request (due to shrunk i_size).

Note that even before writepages patches, the case "Got truncated off
completely" was handled in fuse_send_writepage() by calling
fuse_writepage_finish() which updated BDI_WRITTEN unconditionally.

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 036fcf3..06d6099 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1444,6 +1444,7 @@ static void fuse_drop_writepage(struct fuse_conn *fc, struct fuse_req *req)
 		struct fuse_req *next = req->misc.write.next;
 		dec_bdi_stat(bdi, BDI_WRITEBACK);
 		dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
+		bdi_writeout_inc(bdi);
 		fuse_writepage_free(fc, req);
 		fuse_put_request(fc, req);
 		req = next;
@@ -1736,9 +1737,12 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
 
 	if (page_copied ||
 	    new_req->misc.write.in.offset >= fuse_req_end_pos(old_req)) {
+		struct backing_dev_info *bdi = page->mapping->backing_dev_info;
+
 		spin_unlock(&fc->lock);
-		dec_bdi_stat(page->mapping->backing_dev_info, BDI_WRITEBACK);
+		dec_bdi_stat(bdi, BDI_WRITEBACK);
 		dec_zone_page_state(page, NR_WRITEBACK_TEMP);
+		bdi_writeout_inc(bdi);
 		fuse_writepage_free(fc, new_req);
 		fuse_request_free(new_req);
 		goto out;

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/5] fuse: writepages: protect secondary requests from fuse file release
  2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
                   ` (3 preceding siblings ...)
  2013-10-02 11:02 ` [PATCH 4/5] fuse: writepage: update bdi writeout when deleting secondary request Maxim Patlasov
@ 2013-10-02 11:03 ` Maxim Patlasov
  4 siblings, 0 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:03 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

All async fuse requests must be supplied with extra reference to a fuse file.
This is necessary to ensure that the fuse file is not released until all
in-flight requests are completed. Fuse secondary writeback requests must
obey this rule as well.

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 06d6099..23bc608 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1548,6 +1548,7 @@ static void fuse_writepage_end(struct fuse_conn *fc, struct fuse_req *req)
 		struct fuse_req *next = req->misc.write.next;
 		req->misc.write.next = next->misc.write.next;
 		next->misc.write.next = NULL;
+		next->ff = fuse_file_get(req->ff);
 		list_add(&next->writepages_entry, &fi->writepages);
 		list_add_tail(&next->list, &fi->queued_writes);
 		fuse_flush_writepages(inode);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2
  2013-10-02 11:01 ` [PATCH 2/5] fuse: writepages: crop secondary requests on send Maxim Patlasov
@ 2013-10-02 11:17   ` Maxim Patlasov
  2013-10-02 15:07     ` Miklos Szeredi
  0 siblings, 1 reply; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 11:17 UTC (permalink / raw)
  To: miklos; +Cc: fuse-devel, linux-fsdevel, linux-kernel

If writeback happens while fuse is in FUSE_NOWRITE condition, the request
will be queued but not processed immediately (see fuse_flush_writepages()).
Until FUSE_NOWRITE becomes relaxed, more writebacks can happen. They will
be queued as "secondary" requests to that first ("primary") request.

When FUSE_NOWRITE is relaxed and fuse_send_writepage() is called, it must
crop both primary and secondary requests according to the actual i_size.
Otherwise, if only primary is cropped, an extending write(2) may increase
i_size soon and then secondary requests won't be cropped properly. The result
would be stale data written to the server to a file offset where zeros must be.

Changed in v2:
 - avoid NULL pointer dereference in fuse_drop_writepage().

Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
 fs/fuse/file.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 575e44f..89a2e76 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1435,6 +1435,51 @@ static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)
 	wake_up(&fi->page_waitq);
 }
 
+/* Drop list of secondary writepage requests */
+static void fuse_drop_writepage(struct fuse_conn *fc, struct fuse_req *req)
+{
+	struct backing_dev_info *bdi = req ?
+		req->inode->i_mapping->backing_dev_info : NULL;
+
+	while (req) {
+		struct fuse_req *next = req->misc.write.next;
+		dec_bdi_stat(bdi, BDI_WRITEBACK);
+		dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
+		fuse_writepage_free(fc, req);
+		fuse_put_request(fc, req);
+		req = next;
+	}
+}
+
+/* Crop the misc.write.in.size of parent and secondary writepage requests */
+static bool fuse_crop_writepage(struct fuse_conn *fc, struct fuse_req *req,
+				loff_t size, struct fuse_req **drop_list)
+{
+	if (req->misc.write.in.offset >= size)
+		return true;
+
+	while (req) {
+		struct fuse_req *next = req->misc.write.next;
+		struct fuse_write_in *inarg = &req->misc.write.in;
+		__u64 data_size = inarg->size ? :
+			req->num_pages * PAGE_CACHE_SIZE;
+
+		if (inarg->offset + data_size <= size) {
+			inarg->size = data_size;
+		} else if (inarg->offset < size) {
+			inarg->size = size - inarg->offset;
+		} else {
+			/* Got truncated off completely */
+			req->misc.write.next = *drop_list;
+			*drop_list = req;
+		}
+
+		req = next;
+	}
+
+	return false;
+}
+
 /* Called under fc->lock, may release and reacquire it */
 static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req)
 __releases(fc->lock)
@@ -1443,29 +1488,30 @@ __acquires(fc->lock)
 	struct fuse_inode *fi = get_fuse_inode(req->inode);
 	loff_t size = i_size_read(req->inode);
 	struct fuse_write_in *inarg = &req->misc.write.in;
-	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
+	struct fuse_req *drop_list = NULL;
 
 	if (!fc->connected)
 		goto out_free;
 
-	if (inarg->offset + data_size <= size) {
-		inarg->size = data_size;
-	} else if (inarg->offset < size) {
-		inarg->size = size - inarg->offset;
-	} else {
-		/* Got truncated off completely */
-		goto out_free;
-	}
+	if (fuse_crop_writepage(fc, req, size, &drop_list))
+		goto out_free; /* drop req and descendants */
 
 	req->in.args[1].size = inarg->size;
 	fi->writectr++;
 	fuse_request_send_background_locked(fc, req);
+
+	if (drop_list) {
+		spin_unlock(&fc->lock);
+		fuse_drop_writepage(fc, drop_list);
+		spin_lock(&fc->lock);
+	}
 	return;
 
  out_free:
 	fuse_writepage_finish(fc, req);
 	spin_unlock(&fc->lock);
 	fuse_writepage_free(fc, req);
+	fuse_drop_writepage(fc, req->misc.write.next);
 	fuse_put_request(fc, req);
 	spin_lock(&fc->lock);
 }

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2
  2013-10-02 11:17   ` [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2 Maxim Patlasov
@ 2013-10-02 15:07     ` Miklos Szeredi
  2013-10-02 15:46       ` Maxim Patlasov
  0 siblings, 1 reply; 9+ messages in thread
From: Miklos Szeredi @ 2013-10-02 15:07 UTC (permalink / raw)
  To: Maxim Patlasov; +Cc: fuse-devel, Linux-Fsdevel, Kernel Mailing List

On Wed, Oct 2, 2013 at 1:17 PM, Maxim Patlasov <MPatlasov@parallels.com> wrote:
> If writeback happens while fuse is in FUSE_NOWRITE condition, the request
> will be queued but not processed immediately (see fuse_flush_writepages()).
> Until FUSE_NOWRITE becomes relaxed, more writebacks can happen. They will
> be queued as "secondary" requests to that first ("primary") request.
>
> When FUSE_NOWRITE is relaxed and fuse_send_writepage() is called, it must
> crop both primary and secondary requests according to the actual i_size.
> Otherwise, if only primary is cropped, an extending write(2) may increase
> i_size soon and then secondary requests won't be cropped properly. The result
> would be stale data written to the server to a file offset where zeros must be.
>
> Changed in v2:
>  - avoid NULL pointer dereference in fuse_drop_writepage().
>
> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
> ---
>  fs/fuse/file.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 55 insertions(+), 9 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 575e44f..89a2e76 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1435,6 +1435,51 @@ static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)
>         wake_up(&fi->page_waitq);
>  }
>
> +/* Drop list of secondary writepage requests */
> +static void fuse_drop_writepage(struct fuse_conn *fc, struct fuse_req *req)
> +{
> +       struct backing_dev_info *bdi = req ?
> +               req->inode->i_mapping->backing_dev_info : NULL;
> +
> +       while (req) {
> +               struct fuse_req *next = req->misc.write.next;
> +               dec_bdi_stat(bdi, BDI_WRITEBACK);
> +               dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
> +               fuse_writepage_free(fc, req);
> +               fuse_put_request(fc, req);
> +               req = next;
> +       }
> +}
> +
> +/* Crop the misc.write.in.size of parent and secondary writepage requests */
> +static bool fuse_crop_writepage(struct fuse_conn *fc, struct fuse_req *req,
> +                               loff_t size, struct fuse_req **drop_list)
> +{
> +       if (req->misc.write.in.offset >= size)
> +               return true;
> +
> +       while (req) {
> +               struct fuse_req *next = req->misc.write.next;
> +               struct fuse_write_in *inarg = &req->misc.write.in;
> +               __u64 data_size = inarg->size ? :
> +                       req->num_pages * PAGE_CACHE_SIZE;
> +
> +               if (inarg->offset + data_size <= size) {
> +                       inarg->size = data_size;
> +               } else if (inarg->offset < size) {
> +                       inarg->size = size - inarg->offset;
> +               } else {
> +                       /* Got truncated off completely */
> +                       req->misc.write.next = *drop_list;
> +                       *drop_list = req;

This corrupts the list (the req is not taken off the list before being
added to another).  It could be fixed, but why not check this instead
in fuse_writepage_end() before queuing the next request?

Thanks,
Miklos


> +               }
> +
> +               req = next;
> +       }
> +
> +       return false;
> +}
> +
>  /* Called under fc->lock, may release and reacquire it */
>  static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req)
>  __releases(fc->lock)
> @@ -1443,29 +1488,30 @@ __acquires(fc->lock)
>         struct fuse_inode *fi = get_fuse_inode(req->inode);
>         loff_t size = i_size_read(req->inode);
>         struct fuse_write_in *inarg = &req->misc.write.in;
> -       __u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
> +       struct fuse_req *drop_list = NULL;
>
>         if (!fc->connected)
>                 goto out_free;
>
> -       if (inarg->offset + data_size <= size) {
> -               inarg->size = data_size;
> -       } else if (inarg->offset < size) {
> -               inarg->size = size - inarg->offset;
> -       } else {
> -               /* Got truncated off completely */
> -               goto out_free;
> -       }
> +       if (fuse_crop_writepage(fc, req, size, &drop_list))
> +               goto out_free; /* drop req and descendants */
>
>         req->in.args[1].size = inarg->size;
>         fi->writectr++;
>         fuse_request_send_background_locked(fc, req);
> +
> +       if (drop_list) {
> +               spin_unlock(&fc->lock);
> +               fuse_drop_writepage(fc, drop_list);
> +               spin_lock(&fc->lock);
> +       }
>         return;
>
>   out_free:
>         fuse_writepage_finish(fc, req);
>         spin_unlock(&fc->lock);
>         fuse_writepage_free(fc, req);
> +       fuse_drop_writepage(fc, req->misc.write.next);
>         fuse_put_request(fc, req);
>         spin_lock(&fc->lock);
>  }
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2
  2013-10-02 15:07     ` Miklos Szeredi
@ 2013-10-02 15:46       ` Maxim Patlasov
  0 siblings, 0 replies; 9+ messages in thread
From: Maxim Patlasov @ 2013-10-02 15:46 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: fuse-devel, Linux-Fsdevel, Kernel Mailing List

On 10/02/2013 07:07 PM, Miklos Szeredi wrote:
> On Wed, Oct 2, 2013 at 1:17 PM, Maxim Patlasov <MPatlasov@parallels.com> wrote:
>> If writeback happens while fuse is in FUSE_NOWRITE condition, the request
>> will be queued but not processed immediately (see fuse_flush_writepages()).
>> Until FUSE_NOWRITE becomes relaxed, more writebacks can happen. They will
>> be queued as "secondary" requests to that first ("primary") request.
>>
>> When FUSE_NOWRITE is relaxed and fuse_send_writepage() is called, it must
>> crop both primary and secondary requests according to the actual i_size.
>> Otherwise, if only primary is cropped, an extending write(2) may increase
>> i_size soon and then secondary requests won't be cropped properly. The result
>> would be stale data written to the server to a file offset where zeros must be.
>>
>> Changed in v2:
>>   - avoid NULL pointer dereference in fuse_drop_writepage().
>>
>> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
>> ---
>>   fs/fuse/file.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 55 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 575e44f..89a2e76 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1435,6 +1435,51 @@ static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)
>>          wake_up(&fi->page_waitq);
>>   }
>>
>> +/* Drop list of secondary writepage requests */
>> +static void fuse_drop_writepage(struct fuse_conn *fc, struct fuse_req *req)
>> +{
>> +       struct backing_dev_info *bdi = req ?
>> +               req->inode->i_mapping->backing_dev_info : NULL;
>> +
>> +       while (req) {
>> +               struct fuse_req *next = req->misc.write.next;
>> +               dec_bdi_stat(bdi, BDI_WRITEBACK);
>> +               dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
>> +               fuse_writepage_free(fc, req);
>> +               fuse_put_request(fc, req);
>> +               req = next;
>> +       }
>> +}
>> +
>> +/* Crop the misc.write.in.size of parent and secondary writepage requests */
>> +static bool fuse_crop_writepage(struct fuse_conn *fc, struct fuse_req *req,
>> +                               loff_t size, struct fuse_req **drop_list)
>> +{
>> +       if (req->misc.write.in.offset >= size)
>> +               return true;
>> +
>> +       while (req) {
>> +               struct fuse_req *next = req->misc.write.next;
>> +               struct fuse_write_in *inarg = &req->misc.write.in;
>> +               __u64 data_size = inarg->size ? :
>> +                       req->num_pages * PAGE_CACHE_SIZE;
>> +
>> +               if (inarg->offset + data_size <= size) {
>> +                       inarg->size = data_size;
>> +               } else if (inarg->offset < size) {
>> +                       inarg->size = size - inarg->offset;
>> +               } else {
>> +                       /* Got truncated off completely */
>> +                       req->misc.write.next = *drop_list;
>> +                       *drop_list = req;
> This corrupts the list (the req is not taken off the list before being
> added to another).  It could be fixed, but why not check this instead
> in fuse_writepage_end() before queuing the next request?

Nice idea. This will make next patch ("crop on attach") redundant. I'll 
resend updated patch-set.

Thanks,
Maxim

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-10-02 15:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-02 11:00 [PATCH 0/5] fuse: fixes for fuse_writepage_in_flight() and friends Maxim Patlasov
2013-10-02 11:01 ` [PATCH 1/5] fuse: writepages: roll back changes if request not found Maxim Patlasov
2013-10-02 11:01 ` [PATCH 2/5] fuse: writepages: crop secondary requests on send Maxim Patlasov
2013-10-02 11:17   ` [PATCH 2/5] fuse: writepages: crop secondary requests on send -v2 Maxim Patlasov
2013-10-02 15:07     ` Miklos Szeredi
2013-10-02 15:46       ` Maxim Patlasov
2013-10-02 11:02 ` [PATCH 3/5] fuse: writepages: crop secondary requests on attach Maxim Patlasov
2013-10-02 11:02 ` [PATCH 4/5] fuse: writepage: update bdi writeout when deleting secondary request Maxim Patlasov
2013-10-02 11:03 ` [PATCH 5/5] fuse: writepages: protect secondary requests from fuse file release Maxim Patlasov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).