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
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 <[email protected]>
---
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);
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 <[email protected]>
---
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);
}
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 <[email protected]>
---
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);
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 <[email protected]>
---
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;
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 <[email protected]>
---
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);
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 <[email protected]>
---
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);
}
On Wed, Oct 2, 2013 at 1:17 PM, Maxim Patlasov <[email protected]> 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 <[email protected]>
> ---
> 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);
> }
>
On 10/02/2013 07:07 PM, Miklos Szeredi wrote:
> On Wed, Oct 2, 2013 at 1:17 PM, Maxim Patlasov <[email protected]> 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 <[email protected]>
>> ---
>> 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