Smack on the head: Questions about pnfs write path

DISCUSSION Boaz Harrosh 3 participants 5 messages May 21, 2011 – May 23, 2011

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Boaz Harrosh

author

2011-05-21 23:45 UTC #0 lore raw

Fred git blame points to you:

What was the meaning of the below code:

@@ -1014,18 +1015,22 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc)
*pages++ = req->wb_page;
}

 	req = nfs_list_entry(data->pages.next);
	if ((!lseg) && list_is_singular(&data->pages))

lseg = pnfs_update_layout(desc->pg_inode, req->wb_context,
req_offset(req), desc->pg_count, IOMODE_RW);

I mean why only the list_is_singular() case gets a pnfs_update_layout?

Because I have a very funny BUG:
If I do:

	dd if=/dev/zero of=/mnt/pnfs/dd1 bs=4k count=1
  I get a nice pnfs write out.
But if I do:
	dd if=/dev/zero of=/mnt/pnfs/dd1 bs=4k count=2 (or any > 1)

I see all IO going to MDS

Below patch fixes that:
@@ -1014,18 +1015,22 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc)
*pages++ = req->wb_page;
}
req = nfs_list_entry(data->pages.next);
- if ((!lseg) && list_is_singular(&data->pages))
+ if ((!lseg) /*&& list_is_singular(&data->pages)*/)
lseg = pnfs_update_layout(desc->pg_inode, req->wb_context,
req_offset(req), desc->pg_count, IOMODE_RW);

Which points to what I'm saying all along. pNFS Write path had no testing but by me.

(BTW am still fighting the unbalanced lseg ref)

Thanks
Boaz

Boaz Harrosh

author

2011-05-22 00:16 UTC #1 lore raw

Quoted 5 lines · Boaz Harrosh

Fred git blame points to you:
What was the meaning of the below code:

OK Smack *ME* on the head

A layout driver must have a .pg_test function else that's what happens. pNFS only for a single page. I'd say it should be opposite no?

Boaz

Quoted 42 lines

(quoted diff) +0 -0
@@ -1014,18 +1015,22 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc)
 		*pages++ = req->wb_page;
 	}
 	req = nfs_list_entry(data->pages.next);
 	if ((!lseg) && list_is_singular(&data->pages))
 		lseg = pnfs_update_layout(desc->pg_inode, req->wb_context,
 					  req_offset(req), desc->pg_count,
 					  IOMODE_RW);
 
I mean why only the list_is_singular() case gets a pnfs_update_layout?
Because I have a very funny BUG:
If I do:
	dd if=/dev/zero of=/mnt/pnfs/dd1 bs=4k count=1
  I get a nice pnfs write out.
But if I do:
	dd if=/dev/zero of=/mnt/pnfs/dd1 bs=4k count=2 (or any > 1)
I see all IO going to MDS
Below patch fixes that:
(quoted diff) +1 -1
@@ -1014,18 +1015,22 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc)
 		*pages++ = req->wb_page;
 	}
 	req = nfs_list_entry(data->pages.next);
-	if ((!lseg) && list_is_singular(&data->pages))
+	if ((!lseg) /*&& list_is_singular(&data->pages)*/)
 		lseg = pnfs_update_layout(desc->pg_inode, req->wb_context,
 					  req_offset(req), desc->pg_count,
 					  IOMODE_RW);
 
Which points to what I'm saying all along. pNFS Write path had no testing but by me.
(BTW am still fighting the unbalanced lseg ref)
Thanks
Boaz
--
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

Fred Isaman

2011-05-23 14:22 UTC #2 lore raw

Quoted 4 lines · Boaz Harrosh

Quoted 3 lines · Boaz Harrosh
Fred git blame points to you:

Quoted 9 lines

Quoted 2 lines
What was the meaning of the below code:
OK Smack *ME* on the head
A layout driver must have a .pg_test function else that's what happens. pNFS only for a single page. I'd say it should be opposite no?

Right now, a driver with no pg_test function erroneously misses the pnfs_update_layout call in multi-page io. I'll send a patch to fix that shortly.

Fred

Benny Halevy

2011-05-23 17:50 UTC #3 lore raw

Quoted 10 lines · Fred Isaman

Quoted 9 lines · Boaz Harrosh
Quoted 5 lines · Boaz Harrosh
Fred git blame points to you:
What was the meaning of the below code:
OK Smack *ME* on the head

Quoted 9 lines

Quoted 4 lines
A layout driver must have a .pg_test function else that's what happens. pNFS only for a single page. I'd say it should be opposite no?
Right now, a driver with no pg_test function erroneously misses the pnfs_update_layout call in multi-page io. I'll send a patch to fix that shortly.

After discussing this with Trond how about having pnfs_pageio_init_{read,write} always set

	pgio->pg_test = ld->pg_test

and then let the layout driver initialize its vector to the generic pnfs_{read,write}_pg_test functions, respectively if it requires no layout-type specific code.
(and EXPORT_SYMBOL_GPL them for the layout drivers)

Benny

Quoted 1 line

Fred

Benny Halevy

2011-05-23 17:52 UTC #4 lore raw

Quoted 21 lines · Benny Halevy

Quoted 10 lines · Fred Isaman
Quoted 9 lines · Boaz Harrosh
Quoted 5 lines · Boaz Harrosh
Fred git blame points to you:
What was the meaning of the below code:
OK Smack *ME* on the head
Quoted 9 lines
Quoted 4 lines
A layout driver must have a .pg_test function else that's what happens. pNFS only for a single page. I'd say it should be opposite no?
Right now, a driver with no pg_test function erroneously misses the pnfs_update_layout call in multi-page io. I'll send a patch to fix that shortly.

Quoted 8 lines

After discussing this with Trond how about having
pnfs_pageio_init_{read,write} always set
	pgio->pg_test = ld->pg_test
and then let the layout driver initialize its vector to the generic pnfs_{read,write}_pg_test functions, respectively if it requires no layout-type specific code.
(and EXPORT_SYMBOL_GPL them for the layout drivers)

one more note: pnfs_{read,write}_pg_test should, in this model, return 1 where they call NFS_SERVER(pgio->pg_inode)->pnfs_curr_ld->pg_test today.

Benny

Quoted 8 lines

Benny
Quoted 1 line
Fred
--
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