pnfsblock cleanup and fixes

Also avoid unnecessary lock_page if page is handled by others.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |   78 ++++++++++++++++++++++++++------------
 1 files changed, 54 insertions(+), 24 deletions(-)

fs/nfs/blocklayout/blocklayout.c +0 -0

index 4ced0b0..e8e13f3 100644

fs/nfs/blocklayout/blocklayout.c +34 -24

+
+check_page:
+	/* PageDirty: Other will write this out
+	 * PageWriteback: Other is writing this out
+	 * PageUptodate: It was read before
+	 */
+	if (PageDirty(page) || PageWriteback(page)) {
+		print_page(page);
+		if (locked)
+			unlock_page(page);
+		page_cache_release(page);
+		return NULL;
+	}
+
+	if (!locked) {
+		lock_page(page);
+		if (PageDirty(page) || PageWriteback(page))
+			unlock_page(page);
+		return NULL;
+	}
+	if (!PageUptodate(page)) {
+		/* New page, readin or zero it */
+		init_page_for_write(page, cow_read);
+	}
+	set_page_writeback(page);
+	unlock_page(page);
+
+	return page;
+}
+
 static enum pnfs_try_status
 bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 {

@@ -543,32 +593,12 @@ fill_invalid_ext:
 			dprintk("%s zero %dth page: index %lu isect %llu\n",
 				__func__, npg_zero, index,
 				(unsigned long long)isect);
-			page =
-			    find_or_create_page(wdata->inode->i_mapping, index,
-						GFP_NOFS);
-			if (!page) {
-				dprintk("%s oom\n", __func__);
-				wdata->pnfs_error = -ENOMEM;
+			page = bl_find_get_zeroing_page(wdata->inode, index, cow_read);
+			if (unlikely(IS_ERR(page))) {
+				wdata->pnfs_error = PTR_ERR(page);
 				goto out;
-			}
-
-			/* PageDirty: Other will write this out
-			 * PageWriteback: Other is writing this out
-			 * PageUptodate: It was read before
-			 * sector_initialized: already written out
-			 */
-			if (PageDirty(page) || PageWriteback(page)) {
-				print_page(page);
-				unlock_page(page);
-				page_cache_release(page);
+			} else if (page == NULL)
 				goto next_page;
-			}
-			if (!PageUptodate(page)) {
-				/* New page, readin or zero it */
-				init_page_for_write(page, cow_read);
-			}
-			set_page_writeback(page);
-			unlock_page(page);
 
 			ret = bl_mark_sectors_init(be->be_inval, isect,
 						       PAGE_CACHE_SECTORS);

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Benny Halevy
Sent: Thursday, November 10, 2011 4:32 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 3/8] pnfsblock: move find lock page logic out of bl_write_pagelist

Quoted 5 lines · Peng Tao

Also avoid unnecessary lock_page if page is handled by others.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |   78

++++++++++++++++++++++++++------------

Quoted 7 lines

 1 files changed, 54 insertions(+), 24 deletions(-)

fs/nfs/blocklayout/blocklayout.c +0 -0

index 4ced0b0..e8e13f3 100644

fs/nfs/blocklayout/blocklayout.c cleanup: · line 491

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +5 -0

@@ -484,6 +484,56 @@ cleanup:
 	return ret;
 }
 
+/* Find or create a zeroing page marked being writeback.
+ * Return ERR_PTR on error, NULL to indicate skip this page and page itself
+ * to indicate write out.
+ */
+static struct page*

coding style nit: "page *" rather than "page*"

fs/nfs/blocklayout/blocklayout.c cleanup: · line 495

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.c +4 -0

+bl_find_get_zeroing_page(struct inode *inode, pgoff_t index,
+			struct pnfs_block_extent *cow_read)
+{
+	struct page* page;

checkpatch nit:
ERROR: "foo* bar" should be "foo *bar"

fs/nfs/blocklayout/blocklayout.c cleanup: · line 506

Quoted 12 lines

fs/nfs/blocklayout/blocklayout.c +11 -0

+	int locked = 0;
+	page = find_get_page(inode->i_mapping, index);
+	if (page)
+		goto check_page;
+
+	page = find_or_create_page(inode->i_mapping, index, GFP_NOFS);
+	if (unlikely(!page)) {
+		dprintk("%s oom\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+	locked = 1;

Why not just call find_or_create_page in the first place? dealing with both the locked and unlocked cases here just seems to add unneeded complexity.

Benny

Quoted 60 lines

fs/nfs/blocklayout/blocklayout.c +34 -17

+
+check_page:
+	/* PageDirty: Other will write this out
+	 * PageWriteback: Other is writing this out
+	 * PageUptodate: It was read before
+	 */
+	if (PageDirty(page) || PageWriteback(page)) {
+		print_page(page);
+		if (locked)
+			unlock_page(page);
+		page_cache_release(page);
+		return NULL;
+	}
+
+	if (!locked) {
+		lock_page(page);
+		if (PageDirty(page) || PageWriteback(page))
+			unlock_page(page);
+		return NULL;
+	}
+	if (!PageUptodate(page)) {
+		/* New page, readin or zero it */
+		init_page_for_write(page, cow_read);
+	}
+	set_page_writeback(page);
+	unlock_page(page);
+
+	return page;
+}
+
 static enum pnfs_try_status
 bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 {

@@ -543,32 +593,12 @@ fill_invalid_ext:
 			dprintk("%s zero %dth page: index %lu isect %llu\n",
 				__func__, npg_zero, index,
 				(unsigned long long)isect);
-			page =
-			    find_or_create_page(wdata->inode->i_mapping, index,
-						GFP_NOFS);
-			if (!page) {
-				dprintk("%s oom\n", __func__);
-				wdata->pnfs_error = -ENOMEM;
+			page = bl_find_get_zeroing_page(wdata->inode, index, cow_read);
+			if (unlikely(IS_ERR(page))) {
+				wdata->pnfs_error = PTR_ERR(page);
 				goto out;
-			}
-
-			/* PageDirty: Other will write this out
-			 * PageWriteback: Other is writing this out
-			 * PageUptodate: It was read before
-			 * sector_initialized: already written out
-			 */
-			if (PageDirty(page) || PageWriteback(page)) {
-				print_page(page);
-				unlock_page(page);
-				page_cache_release(page);
+			} else if (page == NULL)
 				goto next_page;

fs/nfs/blocklayout/blocklayout.c fill_invalid_ext: · line 604

Quoted 10 lines

fs/nfs/blocklayout/blocklayout.c +0 -7

-			}
-			if (!PageUptodate(page)) {
-				/* New page, readin or zero it */
-				init_page_for_write(page, cow_read);
-			}
-			set_page_writeback(page);
-			unlock_page(page);
 
 			ret = bl_mark_sectors_init(be->be_inval, isect,
 						       PAGE_CACHE_SECTORS);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 35 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Benny Halevy
Sent: Thursday, November 10, 2011 4:32 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 3/8] pnfsblock: move find lock page logic out of bl_write_pagelist

Quoted 5 lines · Peng Tao

Also avoid unnecessary lock_page if page is handled by others.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |   78

++++++++++++++++++++++++++------------

Quoted 7 lines

 1 files changed, 54 insertions(+), 24 deletions(-)

fs/nfs/blocklayout/blocklayout.c +0 -0

index 4ced0b0..e8e13f3 100644

fs/nfs/blocklayout/blocklayout.c cleanup: · line 491

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +5 -0

@@ -484,6 +484,56 @@ cleanup:
 	return ret;
 }
 
+/* Find or create a zeroing page marked being writeback.
+ * Return ERR_PTR on error, NULL to indicate skip this page and page itself
+ * to indicate write out.
+ */
+static struct page*

coding style nit: "page *" rather than "page*"

Quoted 7 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 495

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.c +4 -0

+bl_find_get_zeroing_page(struct inode *inode, pgoff_t index,
+			struct pnfs_block_extent *cow_read)
+{
+	struct page* page;

checkpatch nit:
ERROR: "foo* bar" should be "foo *bar"

Thank you! Will change both.

Quoted 16 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 506

Quoted 12 lines

fs/nfs/blocklayout/blocklayout.c +11 -0

+	int locked = 0;
+	page = find_get_page(inode->i_mapping, index);
+	if (page)
+		goto check_page;
+
+	page = find_or_create_page(inode->i_mapping, index, GFP_NOFS);
+	if (unlikely(!page)) {
+		dprintk("%s oom\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+	locked = 1;

Why not just call find_or_create_page in the first place? dealing with both the locked and unlocked cases here just seems to add unneeded complexity.

It is to avoid lock page when possible. find_or_create_page() calls find_and_lock_page() but there are cases that the page is already locked by someone (e.g., in nfs_writepage_locked()) and current process has to wait. Pressure tests show many such contention. So I want to optimize it out.

Thanks,
Tao

Quoted 13 lines

Benny

Quoted 10 lines

fs/nfs/blocklayout/blocklayout.c +10 -0

+
+check_page:
+	/* PageDirty: Other will write this out
+	 * PageWriteback: Other is writing this out
+	 * PageUptodate: It was read before
+	 */
+	if (PageDirty(page) || PageWriteback(page)) {
+		print_page(page);
+		if (locked)
+			unlock_page(page);

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 524

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +8 -0

+		page_cache_release(page);
+		return NULL;
+	}
+
+	if (!locked) {
+		lock_page(page);
+		if (PageDirty(page) || PageWriteback(page))
+			unlock_page(page);

Quoted 50 lines

Quoted 50 lines

fs/nfs/blocklayout/blocklayout.c +16 -24

+		return NULL;
+	}
+	if (!PageUptodate(page)) {
+		/* New page, readin or zero it */
+		init_page_for_write(page, cow_read);
+	}
+	set_page_writeback(page);
+	unlock_page(page);
+
+	return page;
+}
+
 static enum pnfs_try_status
 bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 {

@@ -543,32 +593,12 @@ fill_invalid_ext:
 			dprintk("%s zero %dth page: index %lu isect %llu\n",
 				__func__, npg_zero, index,
 				(unsigned long long)isect);
-			page =
-			    find_or_create_page(wdata->inode->i_mapping, index,
-						GFP_NOFS);
-			if (!page) {
-				dprintk("%s oom\n", __func__);
-				wdata->pnfs_error = -ENOMEM;
+			page = bl_find_get_zeroing_page(wdata->inode, index, cow_read);
+			if (unlikely(IS_ERR(page))) {
+				wdata->pnfs_error = PTR_ERR(page);
 				goto out;
-			}
-
-			/* PageDirty: Other will write this out
-			 * PageWriteback: Other is writing this out
-			 * PageUptodate: It was read before
-			 * sector_initialized: already written out
-			 */
-			if (PageDirty(page) || PageWriteback(page)) {
-				print_page(page);
-				unlock_page(page);
-				page_cache_release(page);
+			} else if (page == NULL)
 				goto next_page;
-			}
-			if (!PageUptodate(page)) {
-				/* New page, readin or zero it */
-				init_page_for_write(page, cow_read);
-			}
-			set_page_writeback(page);
-			unlock_page(page);
 

Quoted 6 lines

fs/nfs/blocklayout/blocklayout.c fill_invalid_ext: · line 604

Quoted 2 lines

fs/nfs/blocklayout/blocklayout.c +0 -0

 			ret = bl_mark_sectors_init(be->be_inval, isect,
 						       PAGE_CACHE_SECTORS);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 5:32 PM
To: Peng, Tao
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/8] pnfsblock: move find lock page logic out of bl_write_pagelist

Quoted 3 lines · On 2011-11-10 10:49, [email protected] wrote:

Quoted 3 lines

-----Original Message-----
From: [email protected]

[mailto:[email protected]]

Quoted 58 lines

Quoted 32 lines

On Behalf Of Benny Halevy
Sent: Thursday, November 10, 2011 4:32 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 3/8] pnfsblock: move find lock page logic out of bl_write_pagelist

Quoted 5 lines · Peng Tao

Also avoid unnecessary lock_page if page is handled by others.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |   78

++++++++++++++++++++++++++------------

Quoted 7 lines

 1 files changed, 54 insertions(+), 24 deletions(-)

fs/nfs/blocklayout/blocklayout.c +0 -0

index 4ced0b0..e8e13f3 100644

fs/nfs/blocklayout/blocklayout.c cleanup: · line 491

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +5 -0

@@ -484,6 +484,56 @@ cleanup:
 	return ret;
 }
 
+/* Find or create a zeroing page marked being writeback.
+ * Return ERR_PTR on error, NULL to indicate skip this page and page itself
+ * to indicate write out.
+ */
+static struct page*

coding style nit: "page *" rather than "page*"

Quoted 7 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 495

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.c +4 -0

+bl_find_get_zeroing_page(struct inode *inode, pgoff_t index,
+			struct pnfs_block_extent *cow_read)
+{
+	struct page* page;

checkpatch nit:
ERROR: "foo* bar" should be "foo *bar"

Thank you! Will change both.

Quoted 16 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 506

Quoted 12 lines

fs/nfs/blocklayout/blocklayout.c +11 -0

+	int locked = 0;
+	page = find_get_page(inode->i_mapping, index);
+	if (page)
+		goto check_page;
+
+	page = find_or_create_page(inode->i_mapping, index, GFP_NOFS);
+	if (unlikely(!page)) {
+		dprintk("%s oom\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+	locked = 1;

Why not just call find_or_create_page in the first place? dealing with both the locked and unlocked cases here just seems to add unneeded complexity.

Quoted 1 line

It is to avoid lock page when possible. find_or_create_page() calls

find_and_lock_page() but there are cases that the page is already locked by someone (e.g., in nfs_writepage_locked()) and current process has to wait. Pressure tests show many such contention. So I want to optimize it out.

I see. So isn't there a missing page_cache_release in the late locking case (see below...)

Quoted 16 lines

Thanks,
Tao

Quoted 13 lines

Benny

Quoted 10 lines

fs/nfs/blocklayout/blocklayout.c +10 -0

+
+check_page:
+	/* PageDirty: Other will write this out
+	 * PageWriteback: Other is writing this out
+	 * PageUptodate: It was read before
+	 */
+	if (PageDirty(page) || PageWriteback(page)) {
+		print_page(page);
+		if (locked)
+			unlock_page(page);

Quoted 8 lines

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c cleanup: · line 524

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +8 -0

+		page_cache_release(page);
+		return NULL;
+	}
+
+	if (!locked) {
+		lock_page(page);
+		if (PageDirty(page) || PageWriteback(page))
+			unlock_page(page);

I suspect page_cache_release is required here too. Actually, doing the following instead makes sense to me:

	if (!locked) {
		lock_page(page);
		locked = true;	// yeah, make it should be bool, not int
		goto check_page;
	}

Benny

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

fs/nfs/blocklayout/blocklayout.c +12 -0

+		return NULL;
+	}
+	if (!PageUptodate(page)) {
+		/* New page, readin or zero it */
+		init_page_for_write(page, cow_read);
+	}
+	set_page_writeback(page);
+	unlock_page(page);
+
+	return page;
+}
+
 static enum pnfs_try_status
 bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 {

@@ -543,32 +593,12 @@ fill_invalid_ext:
 			dprintk("%s zero %dth page: index %lu isect %llu\n",
 				__func__, npg_zero, index,

Quoted 8 lines

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c fill_invalid_ext:

Quoted 8 lines

fs/nfs/blocklayout/blocklayout.c +1 -6

 				(unsigned long long)isect);
-			page =
-			    find_or_create_page(wdata->inode->i_mapping, index,
-						GFP_NOFS);
-			if (!page) {
-				dprintk("%s oom\n", __func__);
-				wdata->pnfs_error = -ENOMEM;
+			page = bl_find_get_zeroing_page(wdata->inode, index,

cow_read);

Quoted 35 lines

Quoted 24 lines

Quoted 24 lines

fs/nfs/blocklayout/blocklayout.c +3 -18

+			if (unlikely(IS_ERR(page))) {
+				wdata->pnfs_error = PTR_ERR(page);
 				goto out;
-			}
-
-			/* PageDirty: Other will write this out
-			 * PageWriteback: Other is writing this out
-			 * PageUptodate: It was read before
-			 * sector_initialized: already written out
-			 */
-			if (PageDirty(page) || PageWriteback(page)) {
-				print_page(page);
-				unlock_page(page);
-				page_cache_release(page);
+			} else if (page == NULL)
 				goto next_page;
-			}
-			if (!PageUptodate(page)) {
-				/* New page, readin or zero it */
-				init_page_for_write(page, cow_read);
-			}
-			set_page_writeback(page);
-			unlock_page(page);
 

Quoted 6 lines

fs/nfs/blocklayout/blocklayout.c fill_invalid_ext: · line 604

Quoted 2 lines

fs/nfs/blocklayout/blocklayout.c +0 -0

 			ret = bl_mark_sectors_init(be->be_inval, isect,
 						       PAGE_CACHE_SECTORS);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

It is wrong to kmalloc in _add_entry() as it is inside spinlock. memory should be already allocated _add_entry() is called.

So BUG instead if we fail to find a match and no memory preallocated.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/extents.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

fs/nfs/blocklayout/extents.c +0 -0

index f383524..952ea8a 100644
@@ -110,13 +110,8 @@ static int _add_entry(struct my_tree *tree, u64 s, int32_t tag,
 		return 0;

fs/nfs/blocklayout/extents.c +1 -0

+		new = storage;
 		new->it_sector = s;
 		new->it_tags = (1 << tag);
 		list_add(&new->it_link, &pos->it_link);

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 4:44 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 6/8] pnfsblock: clean up _add_entry

Quoted 2 lines · Peng Tao

It is wrong to kmalloc in _add_entry() as it is inside spinlock. memory should be already allocated _add_entry() is called.

What about the call from _set_range, it is passing NULL?

fs/nfs/blocklayout/extents.c static int _add_entry(struct my_tree *tree, u64 s, int32_t

Quoted 13 lines

So BUG instead if we fail to find a match and no memory preallocated.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/extents.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

fs/nfs/blocklayout/extents.c +0 -0

index f383524..952ea8a 100644
@@ -110,13 +110,8 @@ static int _add_entry(struct my_tree *tree, u64 s, int32_t

tag,

fs/nfs/blocklayout/extents.c static int _add_entry(struct my_tree *tree, u64 s, int32_t tag, · line 113

Quoted 11 lines

fs/nfs/blocklayout/extents.c +1 -7

 		return 0;
 	} else {
 		struct pnfs_inval_tracking *new;
-		if (storage)
-			new = storage;
-		else {
-			new = kmalloc(sizeof(*new), GFP_NOFS);
-			if (!new)
-				return -ENOMEM;
-		}
+		BUG_ON(!storage);

The BUG isn't needed as you dereference the pointer right away.

Benny

Quoted 4 lines

fs/nfs/blocklayout/extents.c +1 -0

+		new = storage;
 		new->it_sector = s;
 		new->it_tags = (1 << tag);
 		list_add(&new->it_link, &pos->it_link);

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 4:44 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 6/8] pnfsblock: clean up _add_entry

Quoted 2 lines · Peng Tao

It is wrong to kmalloc in _add_entry() as it is inside spinlock. memory should be already allocated _add_entry() is called.

What about the call from _set_range, it is passing NULL?

fs/nfs/blocklayout/extents.c static int _add_entry(struct my_tree *tree, u64 s, int32_t

Quoted 12 lines

So BUG instead if we fail to find a match and no memory preallocated.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/extents.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

fs/nfs/blocklayout/extents.c +0 -0

index f383524..952ea8a 100644
@@ -110,13 +110,8 @@ static int _add_entry(struct my_tree *tree, u64 s, int32_t

tag,

fs/nfs/blocklayout/extents.c static int _add_entry(struct my_tree *tree, u64 s, int32_t tag, · line 113

Quoted 11 lines

fs/nfs/blocklayout/extents.c +1 -7

 		return 0;
 	} else {
 		struct pnfs_inval_tracking *new;
-		if (storage)
-			new = storage;
-		else {
-			new = kmalloc(sizeof(*new), GFP_NOFS);
-			if (!new)
-				return -ENOMEM;
-		}
+		BUG_ON(!storage);

The BUG isn't needed as you dereference the pointer right away.

Benny

Quoted 4 lines

fs/nfs/blocklayout/extents.c +1 -0

+		new = storage;
 		new->it_sector = s;
 		new->it_tags = (1 << tag);
 		list_add(&new->it_link, &pos->it_link);

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

fs/nfs/blocklayout/blocklayout.h +5 -0

index 60728ac..df0e0fb 100644
@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion */
 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings *marks, sector_t blocksize)
 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 
 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +12 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl,
 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);
+	spin_unlock(&marks->im_lock);

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 4:54 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 7/8] pnfsblock: add im_extents to pnfs_inval_markings

Quoted 9 lines · Peng Tao

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37

+++++++++++++++++++++++++++++++++++++

Quoted 29 lines

 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

fs/nfs/blocklayout/blocklayout.h +0 -0

index 60728ac..df0e0fb 100644

fs/nfs/blocklayout/blocklayout.h struct pnfs_inval_markings { · line 73

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +1 -0

@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion

*/

fs/nfs/blocklayout/blocklayout.h BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

*marks, sector_t blocksize)

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct

Quoted 21 lines

fs/nfs/blocklayout/blocklayout.h +4 -0

 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 
 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +0 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct

pnfs_block_layout *bl,

Quoted 20 lines

fs/nfs/blocklayout/extents.c +17 -0

 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);
+	spin_unlock(&marks->im_lock);
+
+	return 0;
+}
+
+struct pnfs_block_short_extent*

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 887

Quoted 5 lines

fs/nfs/blocklayout/extents.c +5 -0

+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
+	struct pnfs_block_short_extent *rv = NULL;
+
+	if (unlikely(num_to_pop <= 0))
+		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 895

Quoted 9 lines

fs/nfs/blocklayout/extents.c +8 -0

+
+	spin_lock(&marks->im_lock);
+	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
+		rv = list_entry((&marks->im_extents)->next,
+				struct pnfs_block_short_extent, bse_node);
+		list_del_init(&rv->bse_node);
+		if (num_to_pop)
+			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element you pop since you want to return it. This is worth a comment...

I'd consider moving the decrement expression down here or changing the loop to be a for loop to improve its readability. In the latter case this will say if (num_to_pop > 1) kfree(rv) which is more straight forward IMHO.

Benny

Quoted 7 lines

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

Quoted 100 lines

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 4:54 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 7/8] pnfsblock: add im_extents to pnfs_inval_markings

Quoted 9 lines · Peng Tao

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37

+++++++++++++++++++++++++++++++++++++

Quoted 29 lines

 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

fs/nfs/blocklayout/blocklayout.h +0 -0

index 60728ac..df0e0fb 100644

fs/nfs/blocklayout/blocklayout.h struct pnfs_inval_markings { · line 73

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +1 -0

@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion

*/

fs/nfs/blocklayout/blocklayout.h BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

*marks, sector_t blocksize)

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct

Quoted 21 lines

fs/nfs/blocklayout/blocklayout.h +4 -0

 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 
 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +0 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct

pnfs_block_layout *bl,

Quoted 20 lines

fs/nfs/blocklayout/extents.c +17 -0

 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);
+	spin_unlock(&marks->im_lock);
+
+	return 0;
+}
+
+struct pnfs_block_short_extent*

Quoted 8 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 887

Quoted 5 lines

fs/nfs/blocklayout/extents.c +5 -0

+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
+	struct pnfs_block_short_extent *rv = NULL;
+
+	if (unlikely(num_to_pop <= 0))
+		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

Never... I should really replace it with a BUG_ON.

Quoted 12 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 895

Quoted 9 lines

fs/nfs/blocklayout/extents.c +8 -0

+
+	spin_lock(&marks->im_lock);
+	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
+		rv = list_entry((&marks->im_extents)->next,
+				struct pnfs_block_short_extent, bse_node);
+		list_del_init(&rv->bse_node);
+		if (num_to_pop)
+			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element you pop since you want to return it. This is worth a comment...

Yes, you are right. Will add some comments above the function.

Quoted 5 lines

I'd consider moving the decrement expression down here or changing the loop to be a for loop to improve its readability. In the latter case this will say if (num_to_pop > 1) kfree(rv) which is more straight forward IMHO.

How about following?

BUG_ON(num_to_pop <= 0);

list_for_each_entry_safe() {
list_del_init(&rv->bse_node);
if (num_to_pop-- > 1)
kfree(rv);
}

Thanks,
Tao

Quoted 10 lines

Benny

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 902

Quoted 7 lines

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 81 lines · Peng Tao

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

fs/nfs/blocklayout/blocklayout.h +5 -0

index 60728ac..df0e0fb 100644
@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion */
 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings *marks, sector_t blocksize)
 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 
 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +11 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl,
 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 884

Quoted 8 lines

fs/nfs/blocklayout/extents.c +8 -0

+	spin_unlock(&marks->im_lock);
+
+	return 0;
+}
+
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
+	struct pnfs_block_short_extent *rv = NULL;

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 887

Quoted 3 lines

fs/nfs/blocklayout/extents.c +3 -0

+
+	if (unlikely(num_to_pop <= 0))
+		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 895

Quoted 8 lines

fs/nfs/blocklayout/extents.c +8 -0

+
+	spin_lock(&marks->im_lock);
+	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
+		rv = list_entry((&marks->im_extents)->next,
+				struct pnfs_block_short_extent, bse_node);
+		list_del_init(&rv->bse_node);
+		if (num_to_pop)
+			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element you pop since you want to return it. This is worth a comment...

I'd consider moving the decrement expression down here or changing the loop to be a for loop to improve its readability. In the latter case this will say if (num_to_pop > 1) kfree(rv) which is more straight forward IMHO.

Benny

Quoted 7 lines

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 5:21 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 7/8] pnfsblock: add im_extents to pnfs_inval_markings

Quoted 10 lines · Benny Halevy

Quoted 9 lines · Peng Tao

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37

+++++++++++++++++++++++++++++++++++++

Quoted 24 lines

Quoted 24 lines

 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

Quoted 9 lines

fs/nfs/blocklayout/blocklayout.h struct pnfs_inval_markings { · line 73

Quoted 9 lines

fs/nfs/blocklayout/blocklayout.h +1 -0

index 60728ac..df0e0fb 100644
@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion

*/

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

*marks, sector_t blocksize)

Quoted 15 lines

Quoted 15 lines

fs/nfs/blocklayout/blocklayout.h +4 -0

 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 

Quoted 6 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct

Quoted 6 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +0 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct

pnfs_block_layout *bl,

Quoted 17 lines

Quoted 17 lines

fs/nfs/blocklayout/extents.c +14 -0

 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);
+	spin_unlock(&marks->im_lock);
+
+	return 0;

Quoted 5 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 884

Quoted 5 lines

fs/nfs/blocklayout/extents.c +5 -0

+}
+
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
+	struct pnfs_block_short_extent *rv = NULL;

The use case for bl_pop_short_extent comes only in the next patch so it should be introduced there in principle, along with its use.

With that in mind, there is one call site with num_to_pop == 1 in which you don't free any entry, and the other with num_to_pop == num_se which seems to free all the members of the list, but the last one, which is then freed by the caller.

This tells me you crammed two functions into one and you better have one that pops a single element and another to destroy the list.

Benny

Quoted 33 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 887

Quoted 3 lines

fs/nfs/blocklayout/extents.c +3 -0

+
+	if (unlikely(num_to_pop <= 0))
+		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 895

Quoted 8 lines

fs/nfs/blocklayout/extents.c +8 -0

+
+	spin_lock(&marks->im_lock);
+	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
+		rv = list_entry((&marks->im_extents)->next,
+				struct pnfs_block_short_extent, bse_node);
+		list_del_init(&rv->bse_node);
+		if (num_to_pop)
+			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element you pop since you want to return it. This is worth a comment...

I'd consider moving the decrement expression down here or changing the loop to be a for loop to improve its readability. In the latter case this will say if (num_to_pop > 1) kfree(rv) which is more straight forward IMHO.

Benny

Quoted 7 lines

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

Quoted 97 lines

-----Original Message-----
From: Benny Halevy [mailto:[email protected]]
Sent: Thursday, November 10, 2011 5:21 PM
To: Peng Tao
Cc: [email protected]; [email protected]; Peng, Tao
Subject: Re: [PATCH 7/8] pnfsblock: add im_extents to pnfs_inval_markings

Quoted 10 lines · Benny Halevy

Quoted 9 lines · Peng Tao

It stores a list of short extents for INVAL->RW conversion. Also add two functions to manipulate them, in preparation to move malloc logic out of end_io.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |    6 ++++++
 fs/nfs/blocklayout/blocklayout.h |    5 +++++
 fs/nfs/blocklayout/extents.c     |   37

+++++++++++++++++++++++++++++++++++++

Quoted 24 lines

Quoted 24 lines

 3 files changed, 48 insertions(+), 0 deletions(-)

fs/nfs/blocklayout/blocklayout.c +6 -0

index 815c0c3..cb4ff0f 100644
@@ -706,11 +706,17 @@ static void
 release_inval_marks(struct pnfs_inval_markings *marks)
 {
 	struct pnfs_inval_tracking *pos, *temp;
+	struct pnfs_block_short_extent *se, *stemp;
 
 	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
 		list_del(&pos->it_link);
 		kfree(pos);
 	}
+
+	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
+		list_del(&se->bse_node);
+		kfree(se);
+	}
 	return;
 }
 

Quoted 9 lines

fs/nfs/blocklayout/blocklayout.h struct pnfs_inval_markings { · line 73

Quoted 9 lines

fs/nfs/blocklayout/blocklayout.h +1 -0

index 60728ac..df0e0fb 100644
@@ -70,6 +70,7 @@ struct pnfs_inval_markings {
 	spinlock_t	im_lock;
 	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
 	sector_t	im_block_size;	/* Server blocksize in sectors */
+	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion

*/

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

Quoted 4 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 };
 
 struct pnfs_inval_tracking {

@@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings

*marks, sector_t blocksize)

Quoted 15 lines

Quoted 15 lines

fs/nfs/blocklayout/blocklayout.h +4 -0

 {
 	spin_lock_init(&marks->im_lock);
 	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
+	INIT_LIST_HEAD(&marks->im_extents);
 	marks->im_block_size = blocksize;
 	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
 					   blocksize);

@@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
 			sector_t offset, sector_t length);
+int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
 

Quoted 6 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct

Quoted 6 lines

fs/nfs/blocklayout/blocklayout.h +0 -0

 #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */

fs/nfs/blocklayout/extents.c +0 -0

index 952ea8a..72c7fa1 100644
@@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct

pnfs_block_layout *bl,

Quoted 17 lines

Quoted 17 lines

fs/nfs/blocklayout/extents.c +14 -0

 		}
 	}
 }
+
+int
+bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
+	struct pnfs_block_short_extent *new;
+
+	new = kmalloc(sizeof(*new), GFP_NOFS);
+	if (unlikely(!new))
+		return -ENOMEM;
+
+	spin_lock(&marks->im_lock);
+	list_add(&new->bse_node, &marks->im_extents);
+	spin_unlock(&marks->im_lock);
+
+	return 0;

Quoted 8 lines

Quoted 5 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 884

Quoted 5 lines

fs/nfs/blocklayout/extents.c +5 -0

+}
+
+struct pnfs_block_short_extent*
+bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
+	struct pnfs_block_short_extent *rv = NULL;

The use case for bl_pop_short_extent comes only in the next patch so it should be introduced there in principle, along with its use.

I was hoping not to create too large single patch... But since you asked, I will merge them into one.

Quoted 6 lines

With that in mind, there is one call site with num_to_pop == 1 in which you don't free any entry, and the other with num_to_pop == num_se which seems to free all the members of the list, but the last one, which is then freed by the caller.

Quoted 2 lines

This tells me you crammed two functions into one and you better have one that pops a single element and another to destroy the list.

Good point. I thought there might be more users of the function when I first wrote it but apparently I am wrong... I will split it into bl_pop_one_short_extent() and bl_free_short_extents() as you suggested.

Thank you very much!

Tao

Quoted 29 lines

Benny

Quoted 26 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 887

Quoted 3 lines

fs/nfs/blocklayout/extents.c +3 -0

+
+	if (unlikely(num_to_pop <= 0))
+		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 895

Quoted 8 lines

fs/nfs/blocklayout/extents.c +8 -0

+
+	spin_lock(&marks->im_lock);
+	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
+		rv = list_entry((&marks->im_extents)->next,
+				struct pnfs_block_short_extent, bse_node);
+		list_del_init(&rv->bse_node);
+		if (num_to_pop)
+			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element you pop since you want to return it. This is worth a comment...

I'd consider moving the decrement expression down here or changing the loop to be a for loop to improve its readability. In the latter case this will say if (num_to_pop > 1) kfree(rv) which is more straight forward IMHO.

Benny

Quoted 7 lines

Quoted 7 lines

fs/nfs/blocklayout/extents.c clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl, · line 902

Quoted 7 lines

fs/nfs/blocklayout/extents.c +7 -0

+	}
+	spin_unlock(&marks->im_lock);
+
+	BUG_ON(num_to_pop > 0);
+
+	return rv;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

As discussed earlier, it is better for block client to allocate memoroy for

tracking extents state before submitting bio. So the patch does it by allocating a short_extent for every INVALID extent touched by write pagelist and for every zeroing page we created, saving them in layout header.
Then in end_io we can just use them to create commit list items and avoid memory allocation there.

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/blocklayout/blocklayout.c |   70 +++++++++++++++++++++++++++++---------
 fs/nfs/blocklayout/blocklayout.h |    3 +-
 fs/nfs/blocklayout/extents.c     |   13 ++-----
 3 files changed, 60 insertions(+), 26 deletions(-)

fs/nfs/blocklayout/blocklayout.c +41 -16

index cb4ff0f..53cd332 100644
@@ -90,8 +90,9 @@ static int is_writable(struct pnfs_block_extent *be, sector_t isect)
  */
 struct parallel_io {
 	struct kref refcnt;
-	void (*pnfs_callback) (void *data);
+	void (*pnfs_callback) (void *data, int num_se);
 	void *data;
+	int bse_count;
 };
 
 static inline struct parallel_io *alloc_parallel(void *data)

@@ -102,6 +103,7 @@ static inline struct parallel_io *alloc_parallel(void *data)
 	if (rv) {
 		rv->data = data;
 		kref_init(&rv->refcnt);
+		rv->bse_count = 0;
 	}
 	return rv;
 }

@@ -116,7 +118,7 @@ static void destroy_parallel(struct kref *kref)
 	struct parallel_io *p = container_of(kref, struct parallel_io, refcnt);
 
 	dprintk("%s enter\n", __func__);
-	p->pnfs_callback(p->data);
+	p->pnfs_callback(p->data, p->bse_count);
 	kfree(p);
 }
 

@@ -211,7 +213,7 @@ static void bl_read_cleanup(struct work_struct *work)
 }
 
 static void
-bl_end_par_io_read(void *data)
+bl_end_par_io_read(void *data, int unused)
 {
 	struct nfs_read_data *rdata = data;
 

@@ -312,6 +314,7 @@ static void mark_extents_written(struct pnfs_block_layout *bl,
 {
 	sector_t isect, end;
 	struct pnfs_block_extent *be;
+	struct pnfs_block_short_extent *se;
 
 	dprintk("%s(%llu, %u)\n", __func__, offset, count);
 	if (count == 0)

@@ -324,8 +327,11 @@ static void mark_extents_written(struct pnfs_block_layout *bl,
 		be = bl_find_get_extent(bl, isect, NULL);
 		BUG_ON(!be); /* FIXME */
 		len = min(end, be->be_f_offset + be->be_length) - isect;
-		if (be->be_state == PNFS_BLOCK_INVALID_DATA)
-			bl_mark_for_commit(be, isect, len); /* What if fails? */
+		if (be->be_state == PNFS_BLOCK_INVALID_DATA) {
+			se = bl_pop_short_extent(be->be_inval, 1);
+			BUG_ON(!se);
+			bl_mark_for_commit(be, isect, len, se);
+		}
 		isect += len;
 		bl_put_extent(be);
 	}

@@ -347,7 +353,8 @@ static void bl_end_io_write_zero(struct bio *bio, int err)
 		end_page_writeback(page);
 		page_cache_release(page);
 	} while (bvec >= bio->bi_io_vec);
-	if (!uptodate) {
+
+	if (unlikely(!uptodate)) {
 		if (!wdata->pnfs_error)
 			wdata->pnfs_error = -EIO;
 		pnfs_set_lo_fail(wdata->lseg);

@@ -356,7 +363,6 @@ static void bl_end_io_write_zero(struct bio *bio, int err)
 	put_parallel(par);
 }
 
-/* This is basically copied from mpage_end_io_read */
 static void bl_end_io_write(struct bio *bio, int err)
 {
 	struct parallel_io *par = bio->bi_private;

@@ -382,7 +388,7 @@ static void bl_write_cleanup(struct work_struct *work)
 	dprintk("%s enter\n", __func__);
 	task = container_of(work, struct rpc_task, u.tk_work);
 	wdata = container_of(task, struct nfs_write_data, task);
-	if (!wdata->pnfs_error) {
+	if (likely(!wdata->pnfs_error)) {
 		/* Marks for LAYOUTCOMMIT */
 		mark_extents_written(BLK_LSEG2EXT(wdata->lseg),
 				     wdata->args.offset, wdata->args.count);

@@ -391,9 +397,16 @@ static void bl_write_cleanup(struct work_struct *work)
 }
 
 /* Called when last of bios associated with a bl_write_pagelist call finishes */
-static void bl_end_par_io_write(void *data)
+static void bl_end_par_io_write(void *data, int num_se)
 {
 	struct nfs_write_data *wdata = data;
+	struct pnfs_block_short_extent *se;
+
+	if (unlikely(wdata->pnfs_error)) {
+		se = bl_pop_short_extent(&BLK_LSEG2EXT(wdata->lseg)->bl_inval,
+					num_se);
+		kfree(se);
+	}
 
 	wdata->task.tk_status = wdata->pnfs_error;
 	wdata->verf.committed = NFS_FILE_SYNC;

@@ -548,7 +561,7 @@ bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 	 */
 	par = alloc_parallel(wdata);
 	if (!par)
-		return PNFS_NOT_ATTEMPTED;
+		goto out_mds;
 	par->pnfs_callback = bl_end_par_io_write;
 	/* At this point, have to be more careful with error handling */
 

@@ -556,12 +569,15 @@ bl_write_pagelist(struct nfs_write_data *wdata, int sync)
 	be = bl_find_get_extent(BLK_LSEG2EXT(wdata->lseg), isect, &cow_read);
 	if (!be || !is_writable(be, isect)) {
 		dprintk("%s no matching extents!\n", __func__);
-		wdata->pnfs_error = -EINVAL;
-		goto out;
+		goto out_mds;
 	}
 
 	/* First page inside INVALID extent */
 	if (be->be_state == PNFS_BLOCK_INVALID_DATA) {
+		if (likely(!bl_push_one_short_extent(be->be_inval)))
+			par->bse_count++;
+		else
+			goto out_mds;
 		temp = offset >> PAGE_CACHE_SHIFT;
 		npg_zero = do_div(temp, npg_per_block);
 		isect = (sector_t) (((offset - npg_zero * PAGE_CACHE_SIZE) &

@@ -598,6 +614,19 @@ fill_invalid_ext:
 				wdata->pnfs_error = ret;
 				goto out;
 			}
+			if (likely(!bl_push_one_short_extent(be->be_inval)))
+				par->bse_count++;
+			else {
+				end_page_writeback(page);
+				page_cache_release(page);
+				wdata->pnfs_error = -ENOMEM;
+				goto out;
+			}
+			/* FIXME: This should be done in bi_end_io */
+			mark_extents_written(BLK_LSEG2EXT(wdata->lseg),
+					     page->index << PAGE_CACHE_SHIFT,
+					     PAGE_CACHE_SIZE);
+
 			bio = bl_add_page_to_bio(bio, npg_zero, WRITE,
 						 isect, page, be,
 						 bl_end_io_write_zero, par);

@@ -606,10 +635,6 @@ fill_invalid_ext:
 				bio = NULL;
 				goto out;
 			}
-			/* FIXME: This should be done in bi_end_io */
-			mark_extents_written(BLK_LSEG2EXT(wdata->lseg),
-					     page->index << PAGE_CACHE_SHIFT,
-					     PAGE_CACHE_SIZE);
 next_page:
 			isect += PAGE_CACHE_SECTORS;
 			extent_length -= PAGE_CACHE_SECTORS;

fs/nfs/blocklayout/blocklayout.c +11 -0

+				par->bse_count++;
+			    else {
+				wdata->pnfs_error = -ENOMEM;
+				goto out;
+			    }
+			}
 			extent_length = be->be_length -
 			    (isect - be->be_f_offset);
 		}

@@ -680,6 +713,11 @@ out:
 	bl_submit_bio(WRITE, bio);
 	put_parallel(par);
 	return PNFS_ATTEMPTED;
+out_mds:
+	bl_put_extent(be);
+	if (par)
+		kfree(par);
+	return PNFS_NOT_ATTEMPTED;
 }
 
 /* FIXME - range ignored */

fs/nfs/blocklayout/blocklayout.h +2 -1

index df0e0fb..4986c23 100644
@@ -201,7 +201,8 @@ void clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl,
 int bl_add_merge_extent(struct pnfs_block_layout *bl,
 			 struct pnfs_block_extent *new);
 int bl_mark_for_commit(struct pnfs_block_extent *be,
-			sector_t offset, sector_t length);
+			sector_t offset, sector_t length,
+			struct pnfs_block_short_extent *new);
 int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
 struct pnfs_block_short_extent*
 bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);

fs/nfs/blocklayout/extents.c +4 -9

index 72c7fa1..21da3a3 100644
@@ -370,20 +370,18 @@ static void add_to_commitlist(struct pnfs_block_layout *bl,
 
 /* Note the range described by offset, length is guaranteed to be contained
  * within be.
+ * new will be freed, either by this function or add_to_commitlist if they
+ * decide not to use it, or after LAYOUTCOMMIT uses it in the commitlist.
  */
 int bl_mark_for_commit(struct pnfs_block_extent *be,
-		    sector_t offset, sector_t length)
+		    sector_t offset, sector_t length,
+		    struct pnfs_block_short_extent *new)
 {
 	sector_t new_end, end = offset + length;
-	struct pnfs_block_short_extent *new;
 	struct pnfs_block_layout *bl = container_of(be->be_inval,
 						    struct pnfs_block_layout,
 						    bl_inval);
 
-	new = kmalloc(sizeof(*new), GFP_NOFS);
-	if (!new)
-		return -ENOMEM;
-
 	mark_written_sectors(be->be_inval, offset, length);
 	/* We want to add the range to commit list, but it must be
 	 * block-normalized, and verified that the normalized range has

@@ -413,9 +411,6 @@ int bl_mark_for_commit(struct pnfs_block_extent *be,
 	new->bse_mdev = be->be_mdev;
 
 	spin_lock(&bl->bl_ext_lock);
-	/* new will be freed, either by add_to_commitlist if it decides not
-	 * to use it, or after LAYOUTCOMMIT uses it in the commitlist.
-	 */
 	add_to_commitlist(bl, new);
 	spin_unlock(&bl->bl_ext_lock);
 	return 0;

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Peng Tao
Sent: Wednesday, November 09, 2011 11:16 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 0/8] pnfsblock cleanup and fixes

Hi, Trond and Benny,

Bellow are some pnfsblock patches that I have tested for days. Most of them are cleanup and small improvements. Only
[PATCH 2/8] pnfsblock: acquire im_lock in _preload_range
needs to go into stable.

Thanks,
Tao

Peng Tao (8):
pnfsblock: cleanup bl_mark_sectors_init
pnfsblock: acquire im_lock in _preload_range
pnfsblock: move find lock page logic out of bl_write_pagelist
pnfsblock: set read/write tk_status to pnfs_error
pnfsblock: remove rpc_call_ops from struct parallel_io
pnfsblock: clean up _add_entry
pnfsblock: add im_extents to pnfs_inval_markings
pnfsblock: alloc short extent before submit bio

 fs/nfs/blocklayout/blocklayout.c |  176 +++++++++++++++++++++++++-------------
 fs/nfs/blocklayout/blocklayout.h |   11 ++-
 fs/nfs/blocklayout/extents.c     |  146 ++++++++++++--------------------
 3 files changed, 180 insertions(+), 153 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Benny, hi,

These patches fail to apply to pnfs-latest branch because of conflicts with pnfs private workqueue patches. Since Trond has decided not to take pnfs private workqueue patches, would you please drop them so that new patches can apply? I can send you revert patches if you want.

Thanks,
Tao

Quoted 33 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Peng Tao
Sent: Wednesday, November 09, 2011 11:16 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 0/8] pnfsblock cleanup and fixes

Hi, Trond and Benny,

Bellow are some pnfsblock patches that I have tested for days. Most of them are cleanup and small improvements. Only
[PATCH 2/8] pnfsblock: acquire im_lock in _preload_range
needs to go into stable.

Thanks,
Tao

Peng Tao (8):
pnfsblock: cleanup bl_mark_sectors_init
pnfsblock: acquire im_lock in _preload_range
pnfsblock: move find lock page logic out of bl_write_pagelist
pnfsblock: set read/write tk_status to pnfs_error
pnfsblock: remove rpc_call_ops from struct parallel_io
pnfsblock: clean up _add_entry
pnfsblock: add im_extents to pnfs_inval_markings
pnfsblock: alloc short extent before submit bio

 fs/nfs/blocklayout/blocklayout.c |  176 +++++++++++++++++++++++++-------------
 fs/nfs/blocklayout/blocklayout.h |   11 ++-
 fs/nfs/blocklayout/extents.c     |  146 ++++++++++++--------------------
 3 files changed, 180 insertions(+), 153 deletions(-)

Hi, Trond and Benny,

Bellow are some pnfsblock patches that I have tested for days. Most of them are cleanup and small improvements. Only
[PATCH 2/8] pnfsblock: acquire im_lock in _preload_range
needs to go into stable.

Thanks,
Tao

Peng Tao (8):
pnfsblock: cleanup bl_mark_sectors_init
pnfsblock: acquire im_lock in _preload_range

pnfsblock: move find lock page logic out of bl_write_pagelist
pnfsblock: set read/write tk_status to pnfs_error
pnfsblock: remove rpc_call_ops from struct parallel_io
pnfsblock: clean up _add_entry
pnfsblock: add im_extents to pnfs_inval_markings
pnfsblock: alloc short extent before submit bio

 fs/nfs/blocklayout/blocklayout.c |  176 +++++++++++++++++++++++++-------------
 fs/nfs/blocklayout/blocklayout.h |   11 ++-
 fs/nfs/blocklayout/extents.c     |  146 ++++++++++++--------------------
 3 files changed, 180 insertions(+), 153 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Benny Halevy
Sent: Thursday, November 10, 2011 4:38 PM
To: Peng Tao
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 0/8] pnfsblock cleanup and fixes

Quoted 10 lines · Peng Tao

Hi, Trond and Benny,

Bellow are some pnfsblock patches that I have tested for days. Most of them are cleanup and small improvements. Only
[PATCH 2/8] pnfsblock: acquire im_lock in _preload_range
needs to go into stable.

Thanks,
Tao

Quoted 3 lines

Peng Tao (8):
pnfsblock: cleanup bl_mark_sectors_init
pnfsblock: acquire im_lock in _preload_range

I can't find this patch in my mailbox.
It looks like you did not send it...
http://thread.gmane.org/gmane.linux.nfs/44761

Benny

Quoted 8 lines

pnfsblock: move find lock page logic out of bl_write_pagelist
pnfsblock: set read/write tk_status to pnfs_error
pnfsblock: remove rpc_call_ops from struct parallel_io
pnfsblock: clean up _add_entry
pnfsblock: add im_extents to pnfs_inval_markings
pnfsblock: alloc short extent before submit bio

 fs/nfs/blocklayout/blocklayout.c |  176

+++++++++++++++++++++++++-------------

Quoted 8 lines

 fs/nfs/blocklayout/blocklayout.h |   11 ++-
 fs/nfs/blocklayout/extents.c     |  146 ++++++++++++--------------------
 3 files changed, 180 insertions(+), 153 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html