* [PATCH, RFC] xfstests: add d_type checking to fsstress
@ 2013-09-16 21:55 Eric Sandeen
2013-09-16 22:22 ` Mark Tinguely
2013-09-16 22:55 ` Dave Chinner
0 siblings, 2 replies; 5+ messages in thread
From: Eric Sandeen @ 2013-09-16 21:55 UTC (permalink / raw)
To: xfs-oss
This patch adds a "-D" switch to fsstress so that every time
we call readdir, we stat the dentry & compare it's st_mode
to the d_type.
If -D is specified only once, it ignores DT_UNKNOWN. If specified
twice, it considers DT_UNKNOWN to be an error.
It skips paths of "./." and "./.." so that we only look at files
newly created within the filesystem.
This could be used in an xfstest; it's noisy on a failures so
would break expected output.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
fsstress doesn't usually do validation, but it's such a handy
framework for creating a ton of random files, this seems like
an ok place to put it. What do folks think?
diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 5d5611f..711ea9a 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -246,6 +246,7 @@ int rtpct;
unsigned long seed = 0;
ino_t top_ino;
int verbose = 0;
+int verify_d_type = 0;
sig_atomic_t should_stop = 0;
char *execute_cmd = NULL;
int execute_freq = 1;
@@ -315,7 +316,7 @@ int main(int argc, char **argv)
int nousage = 0;
xfs_error_injection_t err_inj;
struct sigaction action;
- const char *allopts = "d:e:f:i:m:M:n:o:p:rs:S:vwx:X:zH";
+ const char *allopts = "d:De:f:i:m:M:n:o:p:rs:S:vwx:X:zH";
errrange = errtag = 0;
umask(0);
@@ -327,6 +328,9 @@ int main(int argc, char **argv)
case 'd':
dirname = optarg;
break;
+ case 'D':
+ verify_d_type++;
+ break;
case 'e':
sscanf(optarg, "%d", &errtag);
if (errtag < 0) {
@@ -1491,6 +1495,7 @@ usage(void)
printf(" [-p nproc][-r len][-s seed][-v][-w][-x cmd][-z][-S][-X ncmd]\n");
printf("where\n");
printf(" -d dir specifies the base directory for operations\n");
+ printf(" -D verify d_type in any readdir operations, 2x to disallow DT_UNKNOWN\n");
printf(" -e errtg specifies error injection stuff\n");
printf(" -f op_name=freq changes the frequency of option name to freq\n");
printf(" the valid operation names are:\n");
@@ -2338,12 +2343,71 @@ getattr_f(int opno, long r)
close(fd);
}
+void test_d_type(int opno, pathname_t *f, struct dirent64 *de)
+{
+ struct stat64 sb;
+ char path[PATH_MAX];
+
+ snprintf(path, PATH_MAX, "%s/%s", f->path, de->d_name);
+
+ /* Don't check ./. or ./.. */
+ if (!strncmp(path, "./.", 3))
+ return;
+
+ if (lstat64(path, &sb)) {
+ printf("%d/%d: getdents - can't stat %s\n",
+ procid, opno, path);
+ } else {
+ int bad_d_type = 0;
+
+ switch (de->d_type) {
+ case DT_BLK:
+ if (!S_ISBLK(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_CHR:
+ if (!S_ISCHR(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_DIR:
+ if (!S_ISDIR(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_FIFO:
+ if (!S_ISFIFO(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_LNK:
+ if (!S_ISLNK(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_REG:
+ if (!S_ISREG(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_SOCK:
+ if (!S_ISSOCK(sb.st_mode))
+ bad_d_type++;
+ break;
+ case DT_UNKNOWN:
+ if (verify_d_type > 1)
+ bad_d_type++;
+ break;
+ }
+ if (bad_d_type)
+ printf("%d/%d: getdents - bad d_type %d for %s\n",
+ procid, opno, de->d_type, path);
+
+ }
+}
+
void
getdents_f(int opno, long r)
{
DIR *dir;
pathname_t f;
int v;
+ struct dirent64 *de;
init_pathname(&f);
if (!get_fname(FT_DIRm, r, &f, NULL, NULL, &v))
@@ -2357,8 +2421,11 @@ getdents_f(int opno, long r)
free_pathname(&f);
return;
}
- while (readdir64(dir) != NULL)
+ while ((de = readdir64(dir)) != NULL) {
+ if (verify_d_type)
+ test_d_type(opno, &f, de);
continue;
+ }
if (v)
printf("%d/%d: getdents %s 0\n", procid, opno, f.path);
free_pathname(&f);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH, RFC] xfstests: add d_type checking to fsstress
2013-09-16 21:55 [PATCH, RFC] xfstests: add d_type checking to fsstress Eric Sandeen
@ 2013-09-16 22:22 ` Mark Tinguely
2013-09-16 22:55 ` Dave Chinner
1 sibling, 0 replies; 5+ messages in thread
From: Mark Tinguely @ 2013-09-16 22:22 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On 09/16/13 16:55, Eric Sandeen wrote:
> This patch adds a "-D" switch to fsstress so that every time
> we call readdir, we stat the dentry& compare it's st_mode
> to the d_type.
>
> If -D is specified only once, it ignores DT_UNKNOWN. If specified
> twice, it considers DT_UNKNOWN to be an error.
>
> It skips paths of "./." and "./.." so that we only look at files
> newly created within the filesystem.
>
> This could be used in an xfstest; it's noisy on a failures so
> would break expected output.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> fsstress doesn't usually do validation, but it's such a handy
> framework for creating a ton of random files, this seems like
> an ok place to put it. What do folks think?
I tried v5 and v4 with and without inode fields in the directory.
This looks good to me. Like the 2 levels of the test.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, RFC] xfstests: add d_type checking to fsstress
2013-09-16 21:55 [PATCH, RFC] xfstests: add d_type checking to fsstress Eric Sandeen
2013-09-16 22:22 ` Mark Tinguely
@ 2013-09-16 22:55 ` Dave Chinner
2013-09-16 23:20 ` Eric Sandeen
2013-09-17 0:54 ` Eric Sandeen
1 sibling, 2 replies; 5+ messages in thread
From: Dave Chinner @ 2013-09-16 22:55 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Mon, Sep 16, 2013 at 04:55:28PM -0500, Eric Sandeen wrote:
> This patch adds a "-D" switch to fsstress so that every time
> we call readdir, we stat the dentry & compare it's st_mode
> to the d_type.
>
> If -D is specified only once, it ignores DT_UNKNOWN. If specified
> twice, it considers DT_UNKNOWN to be an error.
Hmmmm. DT_UNKNOWN is actually a valid type on disk right through to the
userspace interface. I can't think of why we'd want to consider it
invalid, especially as right now xfs_repair siply zeros the field
when recreating directory entries...
Cheers,
Dave.
> +void test_d_type(int opno, pathname_t *f, struct dirent64 *de)
> +{
> + struct stat64 sb;
> + char path[PATH_MAX];
> +
> + snprintf(path, PATH_MAX, "%s/%s", f->path, de->d_name);
> +
> + /* Don't check ./. or ./.. */
> + if (!strncmp(path, "./.", 3))
> + return;
. and .. should have the values of DT_UNKNOWN or DT_DIR. They are
the only valid values for these entries.
> +
> + if (lstat64(path, &sb)) {
> + printf("%d/%d: getdents - can't stat %s\n",
> + procid, opno, path);
> + } else {
> + int bad_d_type = 0;
> +
> + switch (de->d_type) {
> + case DT_BLK:
> + if (!S_ISBLK(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_CHR:
> + if (!S_ISCHR(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_DIR:
> + if (!S_ISDIR(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_FIFO:
> + if (!S_ISFIFO(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_LNK:
> + if (!S_ISLNK(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_REG:
> + if (!S_ISREG(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_SOCK:
> + if (!S_ISSOCK(sb.st_mode))
> + bad_d_type++;
> + break;
> + case DT_UNKNOWN:
> + if (verify_d_type > 1)
> + bad_d_type++;
> + break;
> + }
And DT_WHT? That's defined on disk and in the user interface ;)
i.e. this will not do the right thing with an unknown de->d_type.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, RFC] xfstests: add d_type checking to fsstress
2013-09-16 22:55 ` Dave Chinner
@ 2013-09-16 23:20 ` Eric Sandeen
2013-09-17 0:54 ` Eric Sandeen
1 sibling, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2013-09-16 23:20 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss
On 9/16/13 5:55 PM, Dave Chinner wrote:
> On Mon, Sep 16, 2013 at 04:55:28PM -0500, Eric Sandeen wrote:
>> This patch adds a "-D" switch to fsstress so that every time
>> we call readdir, we stat the dentry & compare it's st_mode
>> to the d_type.
>>
>> If -D is specified only once, it ignores DT_UNKNOWN. If specified
>> twice, it considers DT_UNKNOWN to be an error.
>
> Hmmmm. DT_UNKNOWN is actually a valid type on disk right through to the
> userspace interface. I can't think of why we'd want to consider it
> invalid, especially as right now xfs_repair siply zeros the field
> when recreating directory entries...
well, I didn't know that last part. (but why do that?)
I was thinking along the lines of DT_UNKNOWN returns meaning
"this fs doesn't support d_type."
"If the file type could not be determined, the value DT_UNKNOWN is returned in d_type."
What d_type-grokking fs can't determine its file types?
-Eric
> Cheers,
>
> Dave.
>
>> +void test_d_type(int opno, pathname_t *f, struct dirent64 *de)
>> +{
>> + struct stat64 sb;
>> + char path[PATH_MAX];
>> +
>> + snprintf(path, PATH_MAX, "%s/%s", f->path, de->d_name);
>> +
>> + /* Don't check ./. or ./.. */
>> + if (!strncmp(path, "./.", 3))
>> + return;
>
> . and .. should have the values of DT_UNKNOWN or DT_DIR. They are
> the only valid values for these entries.
>
>> +
>> + if (lstat64(path, &sb)) {
>> + printf("%d/%d: getdents - can't stat %s\n",
>> + procid, opno, path);
>> + } else {
>> + int bad_d_type = 0;
>> +
>> + switch (de->d_type) {
>> + case DT_BLK:
>> + if (!S_ISBLK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_CHR:
>> + if (!S_ISCHR(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_DIR:
>> + if (!S_ISDIR(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_FIFO:
>> + if (!S_ISFIFO(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_LNK:
>> + if (!S_ISLNK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_REG:
>> + if (!S_ISREG(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_SOCK:
>> + if (!S_ISSOCK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_UNKNOWN:
>> + if (verify_d_type > 1)
>> + bad_d_type++;
>> + break;
>> + }
>
> And DT_WHT? That's defined on disk and in the user interface ;)
>
> i.e. this will not do the right thing with an unknown de->d_type.
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, RFC] xfstests: add d_type checking to fsstress
2013-09-16 22:55 ` Dave Chinner
2013-09-16 23:20 ` Eric Sandeen
@ 2013-09-17 0:54 ` Eric Sandeen
1 sibling, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2013-09-17 0:54 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss
On 9/16/13 5:55 PM, Dave Chinner wrote:
> On Mon, Sep 16, 2013 at 04:55:28PM -0500, Eric Sandeen wrote:
>> This patch adds a "-D" switch to fsstress so that every time
>> we call readdir, we stat the dentry & compare it's st_mode
>> to the d_type.
>>
>> If -D is specified only once, it ignores DT_UNKNOWN. If specified
>> twice, it considers DT_UNKNOWN to be an error.
>
> Hmmmm. DT_UNKNOWN is actually a valid type on disk right through to the
> userspace interface. I can't think of why we'd want to consider it
> invalid, especially as right now xfs_repair siply zeros the field
> when recreating directory entries...
>
> Cheers,
>
> Dave.
no fair signing off w/ more text below ;)
>> +void test_d_type(int opno, pathname_t *f, struct dirent64 *de)
>> +{
>> + struct stat64 sb;
>> + char path[PATH_MAX];
>> +
>> + snprintf(path, PATH_MAX, "%s/%s", f->path, de->d_name);
>> +
>> + /* Don't check ./. or ./.. */
>> + if (!strncmp(path, "./.", 3))
>> + return;
>
> . and .. should have the values of DT_UNKNOWN or DT_DIR. They are
> the only valid values for these entries.
Hm let me look at something, I saw something that prompted this
but now that I think about it maybe it's a bug.
>> +
>> + if (lstat64(path, &sb)) {
>> + printf("%d/%d: getdents - can't stat %s\n",
>> + procid, opno, path);
>> + } else {
>> + int bad_d_type = 0;
>> +
>> + switch (de->d_type) {
>> + case DT_BLK:
>> + if (!S_ISBLK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_CHR:
>> + if (!S_ISCHR(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_DIR:
>> + if (!S_ISDIR(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_FIFO:
>> + if (!S_ISFIFO(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_LNK:
>> + if (!S_ISLNK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_REG:
>> + if (!S_ISREG(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_SOCK:
>> + if (!S_ISSOCK(sb.st_mode))
>> + bad_d_type++;
>> + break;
>> + case DT_UNKNOWN:
>> + if (verify_d_type > 1)
>> + bad_d_type++;
>> + break;
>> + }
>
> And DT_WHT? That's defined on disk and in the user interface ;)
but fsstress won't create it, will it?
> i.e. this will not do the right thing with an unknown de->d_type.
... but we know what fsstress can possibly create, right, so testing
those created types should be safe, I'd think.
I'll give this some thought & send V2.
-Eric
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-17 0:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-16 21:55 [PATCH, RFC] xfstests: add d_type checking to fsstress Eric Sandeen
2013-09-16 22:22 ` Mark Tinguely
2013-09-16 22:55 ` Dave Chinner
2013-09-16 23:20 ` Eric Sandeen
2013-09-17 0:54 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox