Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-25 Thread Niels de Vos 
On Mon, Jul 25, 2016 at 05:43:30AM +0200, Emmanuel Dreyfus wrote:
> Vijay Bellur  wrote:
> 
> > Do you have any concrete examples of problems encountered due to the 
> > same directory stream being invoked from multiple threads?
> 
> I am not sure this scenario can happen, but what we had were directory
> offsets reused among different DIR * opened on the same directory. This
> works on Linux but is a standard violation, as directory offsets are
> supposed to be valid only for a given DIR *.
> 
> It broke NetBSD regression enough that I added a test against it in
> xlator/storage/posix/src:posix.c
> 
> seekdir (dir, off); 
> #ifndef GF_LINUX_HOST_OS
> if ((u_long)telldir(dir) != off && off != pfd->dir_eof)
> {
> gf_msg (THIS->name, GF_LOG_ERROR, EINVAL,
> P_MSG_DIR_OPERATION_FAILED,
> "seekdir(0x%llx) failed on dir=%p: "
> "Invalid argument (offset reused from "
> "another DIR * structure?)", off, dir);
> errno = EINVAL; 
> count = -1;
> goto out;
> }
> #endif /* GF_LINUX_HOST_OS */
> 
> 
> 
> About standards and portability, here is the relevant part in Linux man
> page: 
> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not
> > required to be thread-safe.  However, in modern implementations
> > (including the glibc implementation), concurrent calls to readdir(3)
> > that specify different directory streams are thread-safe.
> > 
> > It is expected that a future version of POSIX.1 will make readdir_r()
> > obsolete, and require that readdir() be thread-safe when concurrently
> > employed on different directory streams.
> 
> This means linux recommands using readir(), but such practice is likely
> to break on other systems, since standards do not currently requires it
> to be thread-safe. We can go the readdir() way, but pleas add locks.
> 
> Alternatively we can use #ifdef to use alternate code on Linux (readdir)
> and others (readdir_r)

This 2nd approach is indeed what Kaleb implemented in
http://review.gluster.org/14838 and has mentioned in an other reply in
the thread.

HTH,
Niels


signature.asc
Description: PGP signature
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-25 Thread Kaleb S. KEITHLEY 
On 07/25/2016 07:26 AM, Kaleb S. KEITHLEY wrote:
> On 07/23/2016 10:32 AM, Emmanuel Dreyfus wrote:
>> Pranith Kumar Karampuri  wrote:
>>
>>> So should we do readdir() with external locks for everything instead?
>>
>> readdir() with a per-directory lock is safe. However, it may come with a
>> performance hit in some scenarios, since two threads cannot read the
>> same directory at once. But I am not sure it can happen in GlusterFS.
>>
>> I am a bit disturbed by readdir_r() being planned for deprecation. The
>> Open Group does not say that, or I missed it:
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
> 
> You should take that concern up, perhaps, with the glibc people.
> 
> As for GlusterFS, the recent change I made only affects Linux/glibc.
> 
> Non-linux platforms are unchanged; they use the same old hodgepodge of
> readdir(3)/readdir_r(3) they always have.

I take that back. Non-linux platforms now use readdir_r(3) exclusively.
Which seems to me to be better than the old hodgepodge of readdir(3) and
readdir_r(3).

> 
> As such I don't understand what it is that you're concerned about.
> 

-- 

Kaleb
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-25 Thread Kaleb S. KEITHLEY 
On 07/23/2016 10:32 AM, Emmanuel Dreyfus wrote:
> Pranith Kumar Karampuri  wrote:
> 
>> So should we do readdir() with external locks for everything instead?
> 
> readdir() with a per-directory lock is safe. However, it may come with a
> performance hit in some scenarios, since two threads cannot read the
> same directory at once. But I am not sure it can happen in GlusterFS.
> 
> I am a bit disturbed by readdir_r() being planned for deprecation. The
> Open Group does not say that, or I missed it:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html

You should take that concern up, perhaps, with the glibc people.

As for GlusterFS, the recent change I made only affects Linux/glibc.

Non-linux platforms are unchanged; they use the same old hodgepodge of
readdir(3)/readdir_r(3) they always have.

As such I don't understand what it is that you're concerned about.

-- 

Kaleb
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-24 Thread Emmanuel Dreyfus 
Vijay Bellur  wrote:

> Do you have any concrete examples of problems encountered due to the 
> same directory stream being invoked from multiple threads?

I am not sure this scenario can happen, but what we had were directory
offsets reused among different DIR * opened on the same directory. This
works on Linux but is a standard violation, as directory offsets are
supposed to be valid only for a given DIR *.

It broke NetBSD regression enough that I added a test against it in
xlator/storage/posix/src:posix.c

seekdir (dir, off); 
#ifndef GF_LINUX_HOST_OS
if ((u_long)telldir(dir) != off && off != pfd->dir_eof)
{
gf_msg (THIS->name, GF_LOG_ERROR, EINVAL,
P_MSG_DIR_OPERATION_FAILED,
"seekdir(0x%llx) failed on dir=%p: "
"Invalid argument (offset reused from "
"another DIR * structure?)", off, dir);
errno = EINVAL; 
count = -1;
goto out;
}
#endif /* GF_LINUX_HOST_OS */



About standards and portability, here is the relevant part in Linux man
page: 
> In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not
> required to be thread-safe.  However, in modern implementations
> (including the glibc implementation), concurrent calls to readdir(3)
> that specify different directory streams are thread-safe.
> 
> It is expected that a future version of POSIX.1 will make readdir_r()
> obsolete, and require that readdir() be thread-safe when concurrently
> employed on different directory streams.

This means linux recommands using readir(), but such practice is likely
to break on other systems, since standards do not currently requires it
to be thread-safe. We can go the readdir() way, but pleas add locks.

Alternatively we can use #ifdef to use alternate code on Linux (readdir)
and others (readdir_r)
-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-23 Thread Pranith Kumar Karampuri 
On Sat, Jul 23, 2016 at 8:02 PM, Emmanuel Dreyfus  wrote:

> Pranith Kumar Karampuri  wrote:
>
> > So should we do readdir() with external locks for everything instead?
>
> readdir() with a per-directory lock is safe. However, it may come with a
> performance hit in some scenarios, since two threads cannot read the
> same directory at once. But I am not sure it can happen in GlusterFS.
>
> I am a bit disturbed by readdir_r() being planned for deprecation. The
> Open Group does not say that, or I missed it:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html


I will wait for more people to comment on this. Let us see what they think
as well.


>
>
> --
> Emmanuel Dreyfus
> http://hcpnet.free.fr/pubz
> m...@netbsd.org
>



-- 
Pranith
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-23 Thread Emmanuel Dreyfus 
Pranith Kumar Karampuri  wrote:

> So should we do readdir() with external locks for everything instead?

readdir() with a per-directory lock is safe. However, it may come with a
performance hit in some scenarios, since two threads cannot read the
same directory at once. But I am not sure it can happen in GlusterFS.

I am a bit disturbed by readdir_r() being planned for deprecation. The
Open Group does not say that, or I missed it:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-07-23 Thread Pranith Kumar Karampuri 
Emmanuel,
   I procrastinated too long on this :-/, It is July already :-(. I
just looked at the man page in Linux and it is a bit confusing, so I am not
sure how to go ahead.

For readdir_r(), I see:

DESCRIPTION
   This function is deprecated; use readdir(3) instead.

   The readdir_r() function was invented as a reentrant version of
readdir(3).  It reads
   the next directory entry from the directory stream dirp, and returns
it in the  call‐
   er-allocated  buffer  pointed  to by entry.  For details of the
dirent structure, see
   readir(3).

For readdir(3) I see:
ATTRIBUTES
   For an explanation of the terms used in this section, see
attributes(7).

   ┌──┬───┬──┐
   │Interface │ Attribute │ Value│
   ├──┼───┼──┤
   │readdir() │ Thread safety │ MT-Unsafe race:dirstream │
   └──┴───┴──┘

   In the current POSIX.1 specification (POSIX.1-2008), readdir() is
not required to be thread-safe.  However, in modern implementations
(including the glibc implementation), concur‐
   rent  calls  to readdir() that specify different directory streams
are thread-safe.  In cases where multiple threads must read from the same
directory stream, using readdir() with
   external synchronization is still preferable to the use of the
deprecated readdir_r(3) function.  It is expected that a future version of
POSIX.1 will require  that  readdir()  be
   thread-safe when concurrently employed on different directory
streams.


So should we do readdir() with external locks for everything instead?


On Thu, Feb 11, 2016 at 2:35 PM, Emmanuel Dreyfus  wrote:

> Juste to make sure there is no misunderstanding here: unfortunately I
> do not have time right now to submit a fix. It would be nice if someone
> else coule look at it.
>
> On Wed, Feb 10, 2016 at 01:48:52PM +, Emmanuel Dreyfus wrote:
> > Hi
> >
> > After obtaining a core in a regression, I noticed there are a few
> readdir()
> > use in threaded code. This is begging for a crash, as readdir() maintains
> > an internal state that will be trashed on concurent use. readdir_r()
> > should be used instead.
> >
> > A quick search shows readdir(à usage here:
> > contrib/fuse-util/mount_util.c:30
> > extras/test/ld-preload-test/ld-preload-test.c:310
> > extras/test/test-ffop.c:550
> > libglusterfs/src/compat.c:256
> > libglusterfs/src/compat.c:315
> > libglusterfs/src/syscall.c:97
> > tests/basic/fops-sanity.c:662
> > tests/utils/arequal-checksum.c:331
> >
> > Occurences in contrib, extra and tests are probably harmless are there
> > are usage in standalone programs that are not threaded. We are left with
> > three groups of problems:
> >
> > 1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315
> > This is Solaris compatibility code. Is it used at all?
> >
> > 2)  libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper,
> > which is in turn used in:
> > libglusterfs/src/run.c:284
> > xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582
> > xlators/features/changelog/lib/src/gf-history-changelog.c:854
> > xlators/features/index/src/index.c:471
> > xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
> > xlators/storage/posix/src/posix.c:3700
> > xlators/storage/posix/src/posix.c:5896
> >
> > 3) We also find sys_readdir() in libglusterfs/src/common-utils.h for
> > GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in:
> > libglusterfs/src/common-utils.c:3979
> > libglusterfs/src/common-utils.c:4002
> > xlators/mgmt/glusterd/src/glusterd-hooks.c:365
> > xlators/mgmt/glusterd/src/glusterd-hooks.c:379
> > xlators/mgmt/glusterd/src/glusterd-store.c:651
> > xlators/mgmt/glusterd/src/glusterd-store.c:661
> > xlators/mgmt/glusterd/src/glusterd-store.c:1781
> > xlators/mgmt/glusterd/src/glusterd-store.c:1806
> > xlators/mgmt/glusterd/src/glusterd-store.c:3044
> > xlators/mgmt/glusterd/src/glusterd-store.c:3072
> > xlators/mgmt/glusterd/src/glusterd-store.c:3593
> > xlators/mgmt/glusterd/src/glusterd-store.c:3606
> > xlators/mgmt/glusterd/src/glusterd-store.c:4032
> > xlators/mgmt/glusterd/src/glusterd-store.c:4111
> >
> > There a hive of sprious bugs to squash here.
> >
> > --
> > Emmanuel Dreyfus
> > m...@netbsd.org
> > ___
> > Gluster-devel mailing list
> > Gluster-devel@gluster.org
> > http://www.gluster.org/mailman/listinfo/gluster-devel
>
> --
> Emmanuel Dreyfus
> m...@netbsd.org
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
>



-- 
Pranith
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] readdir() harmful in threaded code

2016-02-11 Thread Emmanuel Dreyfus 
Juste to make sure there is no misunderstanding here: unfortunately I 
do not have time right now to submit a fix. It would be nice if someone
else coule look at it.

On Wed, Feb 10, 2016 at 01:48:52PM +, Emmanuel Dreyfus wrote:
> Hi
> 
> After obtaining a core in a regression, I noticed there are a few readdir()
> use in threaded code. This is begging for a crash, as readdir() maintains
> an internal state that will be trashed on concurent use. readdir_r()
> should be used instead.
> 
> A quick search shows readdir(à usage here:
> contrib/fuse-util/mount_util.c:30
> extras/test/ld-preload-test/ld-preload-test.c:310
> extras/test/test-ffop.c:550
> libglusterfs/src/compat.c:256
> libglusterfs/src/compat.c:315
> libglusterfs/src/syscall.c:97
> tests/basic/fops-sanity.c:662
> tests/utils/arequal-checksum.c:331
> 
> Occurences in contrib, extra and tests are probably harmless are there
> are usage in standalone programs that are not threaded. We are left with
> three groups of problems:
> 
> 1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315
> This is Solaris compatibility code. Is it used at all?
> 
> 2)  libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper, 
> which is in turn used in:
> libglusterfs/src/run.c:284
> xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582
> xlators/features/changelog/lib/src/gf-history-changelog.c:854
> xlators/features/index/src/index.c:471
> xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
> xlators/storage/posix/src/posix.c:3700
> xlators/storage/posix/src/posix.c:5896
> 
> 3) We also find sys_readdir() in libglusterfs/src/common-utils.h for
> GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in:
> libglusterfs/src/common-utils.c:3979
> libglusterfs/src/common-utils.c:4002
> xlators/mgmt/glusterd/src/glusterd-hooks.c:365
> xlators/mgmt/glusterd/src/glusterd-hooks.c:379
> xlators/mgmt/glusterd/src/glusterd-store.c:651
> xlators/mgmt/glusterd/src/glusterd-store.c:661
> xlators/mgmt/glusterd/src/glusterd-store.c:1781
> xlators/mgmt/glusterd/src/glusterd-store.c:1806
> xlators/mgmt/glusterd/src/glusterd-store.c:3044
> xlators/mgmt/glusterd/src/glusterd-store.c:3072
> xlators/mgmt/glusterd/src/glusterd-store.c:3593
> xlators/mgmt/glusterd/src/glusterd-store.c:3606
> xlators/mgmt/glusterd/src/glusterd-store.c:4032
> xlators/mgmt/glusterd/src/glusterd-store.c:4111
> 
> There a hive of sprious bugs to squash here.
> 
> -- 
> Emmanuel Dreyfus
> m...@netbsd.org
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel

-- 
Emmanuel Dreyfus
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

[Gluster-devel] readdir() harmful in threaded code

2016-02-10 Thread Emmanuel Dreyfus 
Hi

After obtaining a core in a regression, I noticed there are a few readdir()
use in threaded code. This is begging for a crash, as readdir() maintains
an internal state that will be trashed on concurent use. readdir_r()
should be used instead.

A quick search shows readdir(à usage here:
contrib/fuse-util/mount_util.c:30
extras/test/ld-preload-test/ld-preload-test.c:310
extras/test/test-ffop.c:550
libglusterfs/src/compat.c:256
libglusterfs/src/compat.c:315
libglusterfs/src/syscall.c:97
tests/basic/fops-sanity.c:662
tests/utils/arequal-checksum.c:331

Occurences in contrib, extra and tests are probably harmless are there
are usage in standalone programs that are not threaded. We are left with
three groups of problems:

1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315
This is Solaris compatibility code. Is it used at all?

2)  libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper, 
which is in turn used in:
libglusterfs/src/run.c:284
xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582
xlators/features/changelog/lib/src/gf-history-changelog.c:854
xlators/features/index/src/index.c:471
xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
xlators/storage/posix/src/posix.c:3700
xlators/storage/posix/src/posix.c:5896

3) We also find sys_readdir() in libglusterfs/src/common-utils.h for
GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in:
libglusterfs/src/common-utils.c:3979
libglusterfs/src/common-utils.c:4002
xlators/mgmt/glusterd/src/glusterd-hooks.c:365
xlators/mgmt/glusterd/src/glusterd-hooks.c:379
xlators/mgmt/glusterd/src/glusterd-store.c:651
xlators/mgmt/glusterd/src/glusterd-store.c:661
xlators/mgmt/glusterd/src/glusterd-store.c:1781
xlators/mgmt/glusterd/src/glusterd-store.c:1806
xlators/mgmt/glusterd/src/glusterd-store.c:3044
xlators/mgmt/glusterd/src/glusterd-store.c:3072
xlators/mgmt/glusterd/src/glusterd-store.c:3593
xlators/mgmt/glusterd/src/glusterd-store.c:3606
xlators/mgmt/glusterd/src/glusterd-store.c:4032
xlators/mgmt/glusterd/src/glusterd-store.c:4111

There a hive of sprious bugs to squash here.

-- 
Emmanuel Dreyfus
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel