Fixed getdevinfo() and dlm_ino_hash() to search for the correct disk name.
Subsystems (2)
PATCH 1/1 Fixed getdevinfo() and dlm_ino_hash() to search for the correct disk name.
The patch "pnfsd: fix test in nfsd4_find_pnfs_dlm_device" was obviously correct; before that patch, nfsd4_find_pnfs_dlm_device() returned the first device which did *not* match, instead of the first device which did.
Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
--- fs/nfsd/nfs4pnfsdlm.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
fs/nfsd/nfs4pnfsdlm.c +9 -3
@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(struct super_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk->disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }
@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , ino->i_sb->s_bdev->bd_disk->disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de)return -1 ; hash_mask = de->num_ds -1 ;
Signature · 1 line
1.7.0.4
Quoted 8 lines · bfields
The patch "pnfsd: fix test in
nfsd4_find_pnfs_dlm_device" was obviously correct; before that patch,nfsd4_find_pnfs_dlm_device()returned the first device which did *not* match, instead of the first device which did.Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
But then I don't know what point the "/dev/" is serving here. May as well just change the interface so that what's passed in is just the name ("sdc2" or whatever)--no use pretending this is some kind of path name if it's not really.
However, I don't know whether passing in the disk name is really a sensible way to refer to a filesystem in a user<->kernel api.
--b.
Quoted 44 lines
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
--- fs/nfsd/nfs4pnfsdlm.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)fs/nfsd/nfs4pnfsdlm.c +9
-3@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(struct super_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk->disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , ino->i_sb->s_bdev->bd_disk->disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de)return -1 ; hash_mask = de->num_ds -1 ;Signature · 1 line
1.7.0.4
Quoted 10 lines · J. Bruce Fields
Quoted 8 lines · bfields
The patch "pnfsd: fix test in
nfsd4_find_pnfs_dlm_device" was obviously correct; before that patch,nfsd4_find_pnfs_dlm_device()returned the first device which did *not* match, instead of the first device which did.Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
Quoted 8 lines
But then I don't know what point the "
/dev/" is serving here. May as well just change the interface so that what's passed in is just the name ("sdc2" or whatever)--no use pretending this is some kind of path name if it's not really.However, I don't know whether passing in the disk name is really a sensible way to refer to a filesystem in a user<->kernel api.
up to you... I think the whole scheme is currently GFS2 specific and it assumes the device exists symmetrically on /dev/... on all DSs. The /dev prefix seems to be completely redundant. It makes more sense to get rid of it in the user->kernel API as passed down to nfsd4_set_pnfs_dlm_device() rather than adding it here.
Benny
Quoted 46 lines
--b.
Quoted 44 lines
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
--- fs/nfsd/nfs4pnfsdlm.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)fs/nfsd/nfs4pnfsdlm.c +9
-3@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(struct super_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk->disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , ino->i_sb->s_bdev->bd_disk->disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de)return -1 ; hash_mask = de->num_ds -1 ;Signature · 1 line
1.7.0.4
Quoted 20 lines · Benny Halevy
Quoted 10 lines · J. Bruce Fields
Quoted 8 lines · bfields
The patch "pnfsd: fix test in
nfsd4_find_pnfs_dlm_device" was obviously correct; before that patch,nfsd4_find_pnfs_dlm_device()returned the first device which did *not* match, instead of the first device which did.Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
Quoted 8 lines
But then I don't know what point the "
/dev/" is serving here. May as well just change the interface so that what's passed in is just the name ("sdc2" or whatever)--no use pretending this is some kind of path name if it's not really.However, I don't know whether passing in the disk name is really a sensible way to refer to a filesystem in a user<->kernel api.
Quoted 6 lines
up to you... I think the whole scheme is currently GFS2 specific and it assumes the device exists symmetrically on
/dev/... on all DSs. The /dev prefix seems to be completely redundant. It makes more sense to get rid of it in theuser->kernelAPI as passed down tonfsd4_set_pnfs_dlm_device()rather than adding it here.
A more through design could use the fs uuid rather than the device name which is kinda volatile...
Benny
Quoted 53 lines
Benny
Quoted 31 lines
--b.
Quoted 29 lines
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
--- fs/nfsd/nfs4pnfsdlm.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)fs/nfsd/nfs4pnfsdlm.c +5
-2@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(struct super_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk->disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }Quoted 15 lines
Quoted 15 lines
fs/nfsd/nfs4pnfsdlm.c +4
-1@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk->disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , ino->i_sb->s_bdev->bd_disk->disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de)return -1 ; hash_mask = de->num_ds -1 ;Signature · 1 line
1.7.0.4--
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 16 lines · Benny Halevy
Quoted 15 lines · J. Bruce Fields
Quoted 8 lines · bfields
The patch "pnfsd: fix test in
nfsd4_find_pnfs_dlm_device" was obviously correct; before that patch,nfsd4_find_pnfs_dlm_device()returned the first device which did *not* match, instead of the first device which did.Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
But then I don't know what point the "
/dev/" is serving here. May as well just change the interface so that what's passed in is just the name ("sdc2" or whatever)--no use pretending this is some kind of path name if it's not really.
Quoted 5 lines
Quoted 3 lines
However, I don't know whether passing in the disk name is really a sensible way to refer to a filesystem in a user<->kernel api.
up to you... I think the whole scheme is currently GFS2 specific
No, it's DLM specific, with GFS2 being the only current user.
Quoted 2 lines
and it assumes the device exists symmetrically on
/dev/... on all DSs.
No. This identifies the MDS exported DLM pNFS file system, and has nothing to do with the local DS file system.
Quoted 6 lines
The /dev prefix seems to be completely redundant. It makes more sense to get rid of it in the
user->kernelAPI as passed down tonfsd4_set_pnfs_dlm_device()rather than adding it here.
Sure.
Quoted 59 lines
Benny
Quoted 44 lines
--b.
Quoted 42 lines
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
---
fs/nfsd/nfs4pnfsdlm.c| 12 +++++++++---1 files changed, 9 insertions(+), 3 deletions(-)fs/nfsd/nfs4pnfsdlm.c +5
-2@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(structsuper_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk- >disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }fs/nfsd/nfs4pnfsdlm.c +3
-1@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk- >disk_name)]; + sprintf(full_disk_name, "/dev/%s", ino->i_sb->s_bdev->bd_disk-Quoted 8 lines
Quoted 8 lines
fs/nfsd/nfs4pnfsdlm.c +1
-0>disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de) return -1; hash_mask = de->num_ds - 1;Signature · 1 line
1.7.0.4--
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
What if we added a struct block_device member to dlm_device_list? That way we could look it up by major:minor numbers.
Quoted 110 lines · Andy Adamson
Quoted 22 lines · Benny Halevy
Quoted 20 lines · J. Bruce Fields
Quoted 11 lines · bfields
The patch "pnfsd: fix test in
nfsd4_find_pnfs_dlm_device" was obviously
correct; before that patch,nfsd4_find_pnfs_dlm_device()returned the
first device which did *not* match, instead of the first device which
did.Other callers previous worked because they passed int the incorrect name, and because we never had more than one element in the list, so returning the first non-match would return the right thing.
But then I don't know what point the "
/dev/" is serving here. May as well just change the interface so that what's passed in is just the name
("sdc2" or whatever)--no use pretending this is some kind of path name
if it's not really.Quoted 5 lines
Quoted 3 lines
However, I don't know whether passing in the disk name is really a sensible way to refer to a filesystem in a user<->kernel api.
up to you... I think the whole scheme is currently GFS2 specific
No, it's DLM specific, with GFS2 being the only current user.
Quoted 2 lines
and it assumes the device exists symmetrically on
/dev/... on all DSs.No. This identifies the MDS exported DLM pNFS file system, and has nothing to do with the local DS file system.
Quoted 6 lines
The /dev prefix seems to be completely redundant. It makes more sense to get rid of it in the
user->kernelAPI as passed down tonfsd4_set_pnfs_dlm_device()rather than adding it here.Sure.
Quoted 60 lines
Benny
Quoted 44 lines
--b.
Quoted 42 lines
But now we need to fix those callers.
From: Eric Anderle <[email protected]>
---
fs/nfsd/nfs4pnfsdlm.c| 12 +++++++++---1 files changed, 9 insertions(+), 3 deletions(-)fs/nfsd/nfs4pnfsdlm.c +5
-2@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(structsuper_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk- >disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }fs/nfsd/nfs4pnfsdlm.c +3
-1@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (ino->i_sb->s_bdev->bd_disk- >disk_name)]; + sprintf(full_disk_name, "/dev/%s", ino->i_sb->s_bdev->bd_disk-Quoted 8 lines
Quoted 8 lines
fs/nfsd/nfs4pnfsdlm.c +1
-0>disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de) return -1; hash_mask = de->num_ds - 1;Signature · 1 line
1.7.0.4--
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
Signature · 2 lines
Eric Anderle <[email protected]> U-M Class of 2013
Quoted 2 lines · Eric Anderle
What if we added a struct
block_devicemember todlm_device_list? That way we could look it up by major:minor numbers.
I don't know.
By the way:
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
fs/nfsd/nfs4pnfsdlm.c +5
-2@@ -268,10 +268,13 @@ static int nfsd4_pnfs_dlm_getdevinfo(structsuper_block *sb,/* * If the DS list has not been established, return -EINVAL */ - dlm_pdev =nfsd4_find_pnfs_dlm_device (sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ +char full_disk_name[6 +strlen (sb->s_bdev->bd_disk->disk_name)]; +sprintf (full_disk_name,"/dev/%s" , sb->s_bdev->bd_disk- >disk_name); + dlm_pdev =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!dlm_pdev) {dprintk ("%s: DEBUG: disk %s Not Found\n" , __func__, - sb->s_bdev->bd_disk->disk_name); + full_disk_name);return err; }fs/nfsd/nfs4pnfsdlm.c +2
-1@@ -364,7 +367,10 @@ static int dlm_ino_hash(struct inode *ino)/* If can't find the inode block device in the pnfs_dlm_deivce list * then don't hand out a layout */ - de =nfsd4_find_pnfs_dlm_device (ino->i_sb->s_bdev->bd_disk- >disk_name); +/* Long enough to hold "/dev/" + disk name */ + char full_disk_name[6 + strlen(ino->i_sb->s_bdev->bd_disk-
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
fs/nfsd/nfs4pnfsdlm.c +2
-0>disk_name)]; +sprintf (full_disk_name,"/dev/%s" , ino->i_sb->s_bdev->bd_disk- >disk_name); + de =nfsd4_find_pnfs_dlm_device (full_disk_name);if (!de) return -1; hash_mask = de->num_ds - 1;
There's some duplicated code there; it should probably go into a common function called from both places.
--b.